Skip to content

Simplify create button copy in modals #2785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
charliepark wants to merge 2 commits into from
Closed

Simplify create button copy in modals #2785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc38d7c
Simplify create button copy
charliepark Apr 7, 2025
d800d46
Some test updates
charliepark Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions app/components/form/SideModalForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { SideModal } from '~/ui/lib/SideModal'
type CreateFormProps = {
formType: 'create'
/** Only needed if you need to override the default button text (`Create ${resourceName}`) */
submitLabel?: string
submitLabel?: 'Add' | 'Assign' | 'Attach' | 'Create' | 'Done' | 'Upload'
}

type EditFormProps = {
Expand Down Expand Up @@ -90,10 +90,7 @@ export function SideModalForm<TFieldValues extends FieldValues>({
}
}, [submitError, form])

const label =
formType === 'edit'
? `Update ${resourceName}`
: submitLabel || title || `Create ${resourceName}`
const label = formType === 'edit' ? 'Update' : submitLabel || title || 'Create'

// must be destructured up here to subscribe to changes. inlining
// form.formState.isDirty does not work
Expand Down
2 changes: 1 addition & 1 deletion app/forms/firewall-rules-create.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export default function CreateFirewallRuleForm() {
}}
loading={updateRules.isPending}
submitError={updateRules.error}
submitLabel="Add rule"
submitLabel="Add"
>
<CommonFields
control={form.control}
Expand Down
2 changes: 1 addition & 1 deletion app/forms/idp/create.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ export default function CreateIdpSideModalForm() {
}}
loading={createIdp.isPending}
submitError={createIdp.error}
submitLabel="Create provider"
submitLabel="Create"
>
<Message
content={
Expand Down
2 changes: 1 addition & 1 deletion app/forms/image-from-snapshot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export default function CreateImageFromSnapshotSideModalForm() {
formType="create"
resourceName="image"
title="Create image from snapshot"
submitLabel="Create image"
submitLabel="Create"
onDismiss={onDismiss}
onSubmit={(body) =>
createImage.mutate({
Expand Down
2 changes: 1 addition & 1 deletion app/forms/image-upload.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ export default function ImageCreate() {
}}
loading={formLoading}
submitError={formError}
submitLabel={allDone ? 'Done' : 'Upload image'}
submitLabel={allDone ? 'Done' : 'Upload'}
>
<Message
variant="info"
Expand Down
2 changes: 1 addition & 1 deletion app/forms/project-access.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function ProjectAccessAddUserSideModal({ onDismiss, policy }: AddRoleModa
}}
loading={updatePolicy.isPending}
submitError={updatePolicy.error}
submitLabel="Assign role"
submitLabel="Assign"
onDismiss={onDismiss}
>
<ListboxField
Expand Down
2 changes: 1 addition & 1 deletion app/forms/silo-access.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export function SiloAccessAddUserSideModal({ onDismiss, policy }: AddRoleModalPr
}}
loading={updatePolicy.isPending}
submitError={updatePolicy.error}
submitLabel="Assign role"
submitLabel="Assign"
Copy link
Collaborator

@david-crespo david-crespo Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of those are harder to understand without the noun.

Copy link
Collaborator

@david-crespo david-crespo Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was my idea to shorten, lol. #2746 (comment). But that was in the context of a much longer one Attach floating IP, which I now see could be shortened to Attach IP. It was also in a PR about a non-side modal form. I think this shorter version might make more sense in the little modal forms because you can see everything right next to the button. With the side modal, I find the form a bit long and the title a bit far away to find a plain ADD intuitive.

Copy link
Contributor Author

@charliepark charliepark Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! Will kill this PR and see about cleaning up just the button copy in free-floating modals.

>
<ListboxField
name="identityId"
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/click-everything.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ test('Click through disks page', async ({ page }) => {
'role=textbox[name="Description"]',
'role=radiogroup[name="Block size (Bytes)"]',
'role=textbox[name="Size (GiB)"]',
'role=button[name="Create disk"]',
'role=button[name="Create"]',
])
await page.goBack()
})
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/disks.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ test.describe('Disk create', () => {
})

test.afterEach(async ({ page }) => {
await page.getByRole('button', { name: 'Create disk' }).click()
await page.getByRole('button', { name: 'Create' }).click()

await expect(page.getByRole('dialog', { name: 'Create disk' })).toBeHidden()
await expectToast(page, 'Disk a-new-disk created')
Expand Down
30 changes: 15 additions & 15 deletions test/e2e/firewall-rules.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,22 +42,22 @@ test('can create firewall rule', async ({ page }) => {

await selectOption(page, 'Target type', 'IP')
await page.getByRole('textbox', { name: 'IP address' }).fill('192.168.0.1')
await page.getByRole('button', { name: 'Add target' }).click()
await page.getByRole('button', { name: 'Add' }).click()
await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' })

// add host filter instance "host-filter-instance"
await selectOption(page, 'Host type', 'Instance')
await page.getByRole('combobox', { name: 'Instance name' }).fill('host-filter-instance')
await page.getByText('host-filter-instance').click()
await page.getByRole('button', { name: 'Add host filter' }).click()
await page.getByRole('button', { name: 'Add' }).click()

// host is added to hosts table
const hosts = page.getByRole('table', { name: 'Host filters' })
await expectRowVisible(hosts, { Type: 'instance', Value: 'host-filter-instance' })

const portRangeField = page.getByRole('textbox', { name: 'Port filters' })
const invalidPort = page.getByRole('dialog').getByText('Not a valid port range')
const addPortButton = page.getByRole('button', { name: 'Add port filter' })
const addPortButton = page.getByRole('button', { name: 'Add' })
await portRangeField.fill('abc')
await expect(invalidPort).toBeHidden()
await addPortButton.click()
Expand Down Expand Up @@ -85,7 +85,7 @@ test('can create firewall rule', async ({ page }) => {
await page.locator('text=UDP').click()

// submit the form
await page.getByRole('button', { name: 'Add rule' }).click()
await page.getByRole('button', { name: 'Add' }).click()

// modal closes again
await expect(modal).toBeHidden()
Expand Down Expand Up @@ -158,7 +158,7 @@ test('firewall rule form targets table', async ({ page }) => {
const targets = page.getByRole('table', { name: 'Targets' })
const targetVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).first()

const addButton = page.getByRole('button', { name: 'Add target' })
const addButton = page.getByRole('button', { name: 'Add' })

// addButton should be disabled until a value is added
await expect(addButton).toBeDisabled()
Expand Down Expand Up @@ -231,7 +231,7 @@ test('firewall rule form targets table', async ({ page }) => {
test('firewall rule form target validation', async ({ page }) => {
await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new')

const addButton = page.getByRole('button', { name: 'Add target' })
const addButton = page.getByRole('button', { name: 'Add' })
const targets = page.getByRole('table', { name: 'Targets' })

const formModal = page.getByRole('dialog', { name: 'Add firewall rule' })
Expand Down Expand Up @@ -296,7 +296,7 @@ test('firewall rule form target validation', async ({ page }) => {
test('firewall rule form host validation', async ({ page }) => {
await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new')

const addButton = page.getByRole('button', { name: 'Add host filter' })
const addButton = page.getByRole('button', { name: 'Add' })
const hosts = page.getByRole('table', { name: 'Host filters' })

const formModal = page.getByRole('dialog', { name: 'Add firewall rule' })
Expand Down Expand Up @@ -368,7 +368,7 @@ test('firewall rule form hosts table', async ({ page }) => {

const hosts = page.getByRole('table', { name: 'Host filters' })
const hostFiltersVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(1)
const addButton = page.getByRole('button', { name: 'Add host filter' })
const addButton = page.getByRole('button', { name: 'Add' })

// add hosts with overlapping names and types to test delete

Expand Down Expand Up @@ -454,14 +454,14 @@ test('can update firewall rule', async ({ page }) => {
await selectOption(page, 'Host type', 'VPC subnet')
await page.getByRole('combobox', { name: 'Subnet name' }).fill('edit-filter-subnet')
await page.getByText('edit-filter-subnet').click()
await page.getByRole('button', { name: 'Add host filter' }).click()
await page.getByRole('button', { name: 'Add' }).click()

// new host is added to hosts table
const hosts = page.getByRole('table', { name: 'Host filters' })
await expectRowVisible(hosts, { Type: 'subnet', Value: 'edit-filter-subnet' })

// submit the form
await page.getByRole('button', { name: 'Update rule' }).click()
await page.getByRole('button', { name: 'Update' }).click()

// modal closes again
await expect(modal).toBeHidden()
Expand Down Expand Up @@ -554,7 +554,7 @@ test('name conflict error on create', async ({ page }) => {
const error = page.getByText('Name taken').first()
await expect(error).toBeHidden()

await page.getByRole('button', { name: 'Add rule' }).click()
await page.getByRole('button', { name: 'Add' }).click()
await expect(error).toBeVisible()
})

Expand All @@ -570,22 +570,22 @@ test('name conflict error on edit', async ({ page }) => {
const error = page.getByRole('dialog').getByText('Name taken')
await expect(error).toBeHidden()

await page.getByRole('button', { name: 'Update rule' }).click()
await page.getByRole('button', { name: 'Update' }).click()
await expect(error).toBeVisible()

// change name back
await nameField.fill('allow-icmp')

// changing a value _without_ changing the name is allowed
await page.getByRole('textbox', { name: 'Priority' }).fill('37')
await page.getByRole('button', { name: 'Update rule' }).click()
await page.getByRole('button', { name: 'Update' }).click()
await expect(error).toBeHidden()
await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp', Priority: '37' })

// changing the name to a non-conflicting name is allowed
await page.getByRole('link', { name: 'allow-icmp' }).click()
await nameField.fill('allow-icmp2')
await page.getByRole('button', { name: 'Update rule' }).click()
await page.getByRole('button', { name: 'Update' }).click()
await expectRowVisible(page.getByRole('table'), { Name: 'allow-icmp2', Priority: '37' })
})

Expand All @@ -609,7 +609,7 @@ test('arbitrary values combobox', async ({ page }) => {
await expectOptions(page, ['Custom: d'])

await vpcInput.blur()
page.getByRole('button', { name: 'Add target' }).click()
page.getByRole('button', { name: 'Add' }).click()
await expect(vpcInput).toHaveValue('')

await vpcInput.focus()
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/floating-ip-create.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ test('can create a floating IP', async ({ page }) => {
// choose pool and submit
await label.click()
await page.getByRole('option', { name: 'ip-pool-1' }).click()
await page.getByRole('button', { name: 'Create floating IP' }).click()
await page.getByRole('button', { name: 'Create' }).click()

await expect(page).toHaveURL(floatingIpsPage)

Expand Down
6 changes: 3 additions & 3 deletions test/e2e/floating-ip-update.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const expectedFormElements = [
'role=heading[name*="Edit floating IP"]',
'role=textbox[name="Name"]',
'role=textbox[name="Description"]',
'role=button[name="Update floating IP"]',
'role=button[name="Update"]',
]

test('can update a floating IP', async ({ page }) => {
Expand All @@ -33,7 +33,7 @@ test('can update a floating IP', async ({ page }) => {

await page.fill('input[name=name]', updatedName)
await page.getByRole('textbox', { name: 'Description' }).fill(updatedDescription)
await page.getByRole('button', { name: 'Update floating IP' }).click()
await page.getByRole('button', { name: 'Update' }).click()
await expect(page).toHaveURL(floatingIpsPage)
await expectRowVisible(page.getByRole('table'), {
name: updatedName,
Expand All @@ -49,7 +49,7 @@ test('can update *just* the floating IP description', async ({ page }) => {
await expectVisible(page, expectedFormElements)

await page.getByRole('textbox', { name: 'Description' }).fill(updatedDescription)
await page.getByRole('button', { name: 'Update floating IP' }).click()
await page.getByRole('button', { name: 'Update' }).click()
await expect(page).toHaveURL(floatingIpsPage)
await expectRowVisible(page.getByRole('table'), {
name: originalName,
Expand Down
20 changes: 10 additions & 10 deletions test/e2e/image-upload.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ test.describe('Image upload', () => {

await fillForm(page, 'new-image')

await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()

// now the modal pops open and the thing starts going
await expectUploadProcess(page)
Expand All @@ -84,12 +84,12 @@ test.describe('Image upload', () => {
await fillForm(page, 'image-1')

await expectNotVisible(page, ['text="Image name already exists"'])
await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()
await expectVisible(page, ['text="Image name already exists"'])

// changing name and resubmitting removes error
await page.fill('role=textbox[name="Name"]', 'image-5')
await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()
await expectNotVisible(page, ['text="Image name already exists"'])
await expectUploadProcess(page)

Expand All @@ -107,7 +107,7 @@ test.describe('Image upload', () => {

await expectNotVisible(page, [nameRequired, fileRequired])

await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()
await expectVisible(page, [nameRequired, fileRequired])

await page.fill('role=textbox[name="Name"]', 'new-image')
Expand All @@ -118,7 +118,7 @@ test.describe('Image upload', () => {
await expectNotVisible(page, [fileRequired])

await page.click('role=button[name="Clear file"]')
await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()

await expectVisible(page, [fileRequired])
})
Expand Down Expand Up @@ -172,7 +172,7 @@ test.describe('Image upload', () => {

const progressModal = page.getByRole('dialog', { name: 'Image upload progress' })

await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()
await expect(progressModal).toBeVisible()

let confirmCount = 0
Expand Down Expand Up @@ -203,7 +203,7 @@ test.describe('Image upload', () => {

await fillForm(page, 'new-image')

await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()

// wait to be in the middle of upload
const uploadStep = page.getByTestId('upload-step: Upload image file')
Expand All @@ -223,10 +223,10 @@ test.describe('Image upload', () => {
await expect(page.getByRole('textbox', { name: 'Name' })).toBeVisible()
// need to wait for submit button to come back because it's in a loading
// state while the cleanup runs
await expect(page.getByRole('button', { name: 'Upload image' })).toBeVisible()
await expect(page.getByRole('button', { name: 'Upload' })).toBeVisible()

// resubmit and it should work fine
await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()
await expectUploadProcess(page)
})

Expand All @@ -244,7 +244,7 @@ test.describe('Image upload', () => {

await fillForm(page, imageName)

await page.getByRole('button', { name: 'Upload image' }).click()
await page.getByRole('button', { name: 'Upload' }).click()

const step = page.getByTestId(`upload-step: ${stepText}`)
await expect(step).toHaveAttribute('data-status', 'error', { timeout: 15000 })
Expand Down
10 changes: 5 additions & 5 deletions test/e2e/instance-create.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,12 +260,12 @@ test('can’t create a disk with a name that collides with the boot disk name',
await page.getByRole('button', { name: 'Create new disk' }).click()
const dialog = page.getByRole('dialog')
await dialog.getByRole('textbox', { name: 'name' }).fill('disk-11')
await dialog.getByRole('button', { name: 'Create disk' }).click()
await dialog.getByRole('button', { name: 'Create' }).click()
// Expect to see an error message
await expect(dialog.getByText('Name is already in use')).toBeVisible()
// Change the disk name to something else
await dialog.getByRole('textbox', { name: 'name' }).fill('disk-12')
await dialog.getByRole('button', { name: 'Create disk' }).click()
await dialog.getByRole('button', { name: 'Create' }).click()
// The disk has been "created" (is in the list of Additional Disks)
await expectVisible(page, ['text=disk-12'])
// Create the instance
Expand Down Expand Up @@ -571,12 +571,12 @@ test('create instance with additional disks', async ({ page }) => {
await sizeField.fill('5')
await expect(sizeField).toHaveValue('5')

await createForm.getByRole('button', { name: 'Create disk' }).click()
await createForm.getByRole('button', { name: 'Create' }).click()
await expect(createForm.getByText('Name is already in use')).toBeVisible()

// rename the disk to one that's allowed
await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
await createForm.getByRole('button', { name: 'Create disk' }).click()
await createForm.getByRole('button', { name: 'Create' }).click()

const disksTable = page.getByRole('table', { name: 'Disks' })
await expect(disksTable.getByText('disk-6')).toBeHidden()
Expand All @@ -585,7 +585,7 @@ test('create instance with additional disks', async ({ page }) => {
// now that name is taken too, so disk create disallows it
await page.getByRole('button', { name: 'Create new disk' }).click()
await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
await createForm.getByRole('button', { name: 'Create disk' }).click()
await createForm.getByRole('button', { name: 'Create' }).click()
await expect(createForm.getByText('Name is already in use')).toBeVisible()
await createForm.getByRole('button', { name: 'Cancel' }).click()

Expand Down
4 changes: 2 additions & 2 deletions test/e2e/instance-disks.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ test('Create disk', async ({ page }) => {
// New disk form
const createForm = page.getByRole('dialog', { name: 'Create disk' })
await expect(createForm).toBeHidden()
await page.getByRole('button', { name: 'Create disk' }).click()
await page.getByRole('button', { name: 'Create' }).click()
await expect(createForm).toBeVisible()

await createForm.getByRole('textbox', { name: 'Name' }).fill('created-disk')
Expand All @@ -120,7 +120,7 @@ test('Create disk', async ({ page }) => {
await page.getByRole('button', { name: 'Source snapshot' }).click()
await page.getByRole('option', { name: 'snapshot-heavy' }).click()

await createForm.getByRole('button', { name: 'Create disk' }).click()
await createForm.getByRole('button', { name: 'Create' }).click()

const otherDisksTable = page.getByRole('table', { name: 'Additional disks' })
await expectRowVisible(otherDisksTable, { Disk: 'created-disk', size: '20 GiB' })
Expand Down
Loading
Loading