From 6378d7d66f37871dc49b1f4cb278e0f457927565 Mon Sep 17 00:00:00 2001 From: Peter Zimon Date: Mon, 1 Jul 2024 13:35:38 +0200 Subject: [PATCH] Unify "Save" and "Close" buttons in Settings (#20430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DES-27 There are two patterns used in settings modals for action buttons: 1. [Cancel] and [Save & close] (sometimes it's [Cancel] and [OK], inconsistently) — example: Staff details, Tier details, Navigation, Recommendation 2. [Close] and [Save] — example: Design settings, Portal, Newsletter details etc. This is confusing and leaves people confused and uncertain about what's going to happen in one or the other case. --- .../settings/general/UserDetailModal.tsx | 7 ++----- .../recommendations/EditRecommendationModal.tsx | 8 ++------ .../membership/tiers/TierDetailModal.tsx | 10 +++------- .../acceptance/general/users/profile.test.ts | 17 +++++++++-------- .../test/acceptance/general/users/roles.test.ts | 5 ++++- .../test/acceptance/membership/tiers.test.ts | 13 ++++++++----- 6 files changed, 28 insertions(+), 32 deletions(-) diff --git a/apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx b/apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx index f44833c327..27e492db49 100644 --- a/apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx +++ b/apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx @@ -113,10 +113,6 @@ const UserDetailModalContent: React.FC<{user: User}> = ({user}) => { onSave: async (values) => { await updateUser?.(values); }, - onSavedStateReset: () => { - mainModal.remove(); - navigateOnClose(); - }, onSaveError: handleError }); const setUserData = (newData: User) => updateForm(() => newData); @@ -353,9 +349,10 @@ const UserDetailModalContent: React.FC<{user: User}> = ({user}) => { animate={canAccessSettings(currentUser)} backDrop={canAccessSettings(currentUser)} buttonsDisabled={okProps.disabled} + cancelLabel='Close' dirty={saveState === 'unsaved'} okColor={okProps.color} - okLabel={okProps.label || 'Save & close'} + okLabel={okProps.label || 'Save'} size={canAccessSettings(currentUser) ? 'lg' : 'bleed'} stickyFooter={true} testId='user-detail-modal' diff --git a/apps/admin-x-settings/src/components/settings/growth/recommendations/EditRecommendationModal.tsx b/apps/admin-x-settings/src/components/settings/growth/recommendations/EditRecommendationModal.tsx index b31d71d772..aa493250e1 100644 --- a/apps/admin-x-settings/src/components/settings/growth/recommendations/EditRecommendationModal.tsx +++ b/apps/admin-x-settings/src/components/settings/growth/recommendations/EditRecommendationModal.tsx @@ -27,10 +27,6 @@ const EditRecommendationModal: React.FC { await editRecommendation(state); }, - onSavedStateReset: () => { - modal.remove(); - updateRoute('recommendations'); - }, onSaveError: handleError, onValidate: (state) => { const newErrors = validateDescriptionForm(state); @@ -76,10 +72,10 @@ const EditRecommendationModal: React.FC> & { const TierDetailModalContent: React.FC<{tier?: Tier}> = ({tier}) => { const isFreeTier = tier?.type === 'free'; - const modal = useModal(); const {updateRoute} = useRouting(); const {mutateAsync: updateTier} = useEditTier(); const {mutateAsync: createTier} = useAddTier(); @@ -97,10 +96,6 @@ const TierDetailModalContent: React.FC<{tier?: Tier}> = ({tier}) => { } } }, - onSavedStateReset: () => { - modal.remove(); - updateRoute('tiers'); - }, onSaveError: handleError }); @@ -185,10 +180,11 @@ const TierDetailModalContent: React.FC<{tier?: Tier}> = ({tier}) => { updateRoute('tiers'); }} buttonsDisabled={okProps.disabled} + cancelLabel='Close' dirty={saveState === 'unsaved'} leftButtonProps={leftButtonProps} okColor={okProps.color} - okLabel={okProps.label || 'Save & close'} + okLabel={okProps.label || 'Save'} size='lg' testId='tier-detail-modal' title={(tier ? (tier.active ? 'Edit tier' : 'Edit archived tier') : 'New tier')} diff --git a/apps/admin-x-settings/test/acceptance/general/users/profile.test.ts b/apps/admin-x-settings/test/acceptance/general/users/profile.test.ts index 46768e1d97..4c7f10ae28 100644 --- a/apps/admin-x-settings/test/acceptance/general/users/profile.test.ts +++ b/apps/admin-x-settings/test/acceptance/general/users/profile.test.ts @@ -35,23 +35,23 @@ test.describe('User profile', async () => { // Validation failures await modal.getByLabel('Full name').fill(''); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal).toContainText('Name is required'); await modal.getByLabel('Email').fill('test'); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal).toContainText('Enter a valid email address'); await modal.getByLabel('Location').fill(new Array(195).join('a')); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal).toContainText('Location is too long'); await modal.getByLabel('Bio').fill(new Array(210).join('a')); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal).toContainText('Bio is too long'); await modal.getByLabel('Website').fill('not-a-website'); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal).toContainText('Enter a valid URL'); const facebookInput = modal.getByLabel('Facebook profile'); @@ -166,9 +166,10 @@ test.describe('User profile', async () => { await modal.getByLabel(/Paid member cancellations/).check(); await modal.getByLabel(/Milestones/).uncheck(); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal.getByRole('button', {name: 'Saved'})).toBeVisible(); + await modal.getByRole('button', {name: 'Close'}).click(); await expect(listItem.getByText('New Admin')).toBeVisible(); await expect(listItem.getByText('newadmin@test.com')).toBeVisible(); @@ -314,7 +315,7 @@ test.describe('User profile', async () => { await modal.getByLabel('Full name').fill('Updated'); - await modal.getByRole('button', {name: 'Cancel'}).click(); + await modal.getByRole('button', {name: 'Close'}).click(); await expect(page.getByTestId('confirmation-modal')).toHaveText(/leave/i); @@ -374,7 +375,7 @@ test.describe('User profile', async () => { await listItem.getByRole('button', {name: 'Edit'}).click(); await expect(modal.getByTestId('api-keys')).toBeHidden(); - await modal.getByRole('button', {name: 'Cancel'}).click(); + await modal.getByRole('button', {name: 'Close'}).click(); // Can see and regenerate your own staff token diff --git a/apps/admin-x-settings/test/acceptance/general/users/roles.test.ts b/apps/admin-x-settings/test/acceptance/general/users/roles.test.ts index 47b30b2b17..67367dc010 100644 --- a/apps/admin-x-settings/test/acceptance/general/users/roles.test.ts +++ b/apps/admin-x-settings/test/acceptance/general/users/roles.test.ts @@ -69,10 +69,12 @@ test.describe('User roles', async () => { await modal.locator('input[value=editor]').check(); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal.getByRole('button', {name: 'Saved'})).toBeVisible(); + await modal.getByRole('button', {name: 'Close'}).click(); + await expect(activeTab).toHaveText(/No authors found./); await section.getByRole('tab', {name: 'Editors'}).click(); @@ -146,6 +148,7 @@ test.describe('User roles', async () => { await modal.getByLabel('Full name').fill('New name'); await modal.getByRole('button', {name: 'Save'}).click(); + await modal.getByRole('button', {name: 'Close'}).click(); await expect(modal).toBeHidden(); }); diff --git a/apps/admin-x-settings/test/acceptance/membership/tiers.test.ts b/apps/admin-x-settings/test/acceptance/membership/tiers.test.ts index 732c8737d6..8a5ecd02b2 100644 --- a/apps/admin-x-settings/test/acceptance/membership/tiers.test.ts +++ b/apps/admin-x-settings/test/acceptance/membership/tiers.test.ts @@ -18,7 +18,7 @@ test.describe('Tier settings', async () => { const modal = page.getByTestId('tier-detail-modal'); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal).toHaveText(/Enter a name for the tier/); await expect(modal).toHaveText(/Amount must be at least \$1/); @@ -51,7 +51,8 @@ test.describe('Tier settings', async () => { browseTiers: {method: 'GET', path: '/tiers/', response: {tiers: [...responseFixtures.tiers.tiers, newTier]}} }}); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); + await modal.getByRole('button', {name: 'Close'}).click(); // await expect(section.getByTestId('tier-card').filter({hasText: /Plus/})).toHaveText(/Plus tier/); // await expect(section.getByTestId('tier-card').filter({hasText: /Plus/})).toHaveText(/\$8\/month/); @@ -103,7 +104,7 @@ test.describe('Tier settings', async () => { // Failing validations await modal.getByLabel('Name').fill(''); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); await expect(modal).toHaveText(/Enter a name for the tier/); @@ -132,7 +133,8 @@ test.describe('Tier settings', async () => { // Save changes - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); + await modal.getByRole('button', {name: 'Close'}).click(); await expect(section.getByTestId('tier-card').filter({hasText: /Supporter/})).toHaveText(/Supporter updated/); await expect(section.getByTestId('tier-card').filter({hasText: /Supporter/})).toHaveText(/Supporter description/); @@ -185,7 +187,8 @@ test.describe('Tier settings', async () => { await modal.getByRole('button', {name: 'Add'}).click(); await modal.getByLabel('New benefit').fill('Second benefit'); - await modal.getByRole('button', {name: 'Save & close'}).click(); + await modal.getByRole('button', {name: 'Save'}).click(); + await modal.getByRole('button', {name: 'Close'}).click(); await expect(section.getByTestId('tier-card').filter({hasText: /Free/})).toHaveText(/Free tier description/);