From d4c3e13d96ec73ce1e8a49ea85890be6c088acdc Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Thu, 11 Jul 2024 12:35:47 -0400 Subject: [PATCH 1/6] update to use new ux --- .../RegionSelect/RegionMultiSelect.tsx | 9 +++---- .../RegionSelect/RegionSelect.types.ts | 2 +- .../ImageRegions/ImageRegionRow.test.tsx | 15 +++++++++++ .../ImageRegions/ImageRegionRow.tsx | 4 ++- .../ImageRegions/ManageImageRegionsForm.tsx | 27 +++++++++---------- 5 files changed, 36 insertions(+), 21 deletions(-) diff --git a/packages/manager/src/components/RegionSelect/RegionMultiSelect.tsx b/packages/manager/src/components/RegionSelect/RegionMultiSelect.tsx index 2d3126e008a..ca9a483438e 100644 --- a/packages/manager/src/components/RegionSelect/RegionMultiSelect.tsx +++ b/packages/manager/src/components/RegionSelect/RegionMultiSelect.tsx @@ -1,4 +1,3 @@ -import { Region } from '@linode/api-v4'; import CloseIcon from '@mui/icons-material/Close'; import React from 'react'; @@ -24,6 +23,7 @@ import type { DisableRegionOption, RegionMultiSelectProps, } from './RegionSelect.types'; +import type { Region } from '@linode/api-v4'; interface LabelComponentProps { region: Region; @@ -67,7 +67,7 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => { selectedIds, sortRegionOptions, width, - onClose, + ...rest } = props; const { @@ -115,7 +115,7 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => { return getRegionCountryGroup(option); }} onChange={(_, selectedOptions) => - onChange(selectedOptions.map((region) => region.id)) + onChange(selectedOptions?.map((region) => region.id) ?? []) } renderOption={(props, option, { selected }) => { if (!option.site_type) { @@ -155,7 +155,6 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => { InputProps: { required, }, - placeholder: selectedRegions.length > 0 ? '' : placeholder, tooltipText: helperText, }} autoHighlight @@ -172,7 +171,7 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => { options={regionOptions} placeholder={placeholder ?? 'Select Regions'} value={selectedRegions} - onClose={onClose} + {...rest} /> {selectedRegions.length > 0 && SelectedRegionsList && ( diff --git a/packages/manager/src/components/RegionSelect/RegionSelect.types.ts b/packages/manager/src/components/RegionSelect/RegionSelect.types.ts index 0b01fc594c0..0ace08a3194 100644 --- a/packages/manager/src/components/RegionSelect/RegionSelect.types.ts +++ b/packages/manager/src/components/RegionSelect/RegionSelect.types.ts @@ -68,7 +68,7 @@ export interface RegionSelectProps< export interface RegionMultiSelectProps extends Omit< - EnhancedAutocompleteProps, + EnhancedAutocompleteProps, 'label' | 'onChange' | 'options' > { SelectedRegionsList?: React.ComponentType<{ diff --git a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.test.tsx b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.test.tsx index 7c8d8bd17f8..1e5bd6bb30d 100644 --- a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.test.tsx +++ b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.test.tsx @@ -39,4 +39,19 @@ describe('ImageRegionRow', () => { expect(onRemove).toHaveBeenCalled(); }); + + it('disables the remove button if disableRemoveButton is true', async () => { + const { getByLabelText } = renderWithTheme( + + ); + + const removeButton = getByLabelText('Remove us-east'); + + expect(removeButton).toBeDisabled(); + }); }); diff --git a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.tsx b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.tsx index a3a1ccd292b..2479578d43c 100644 --- a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.tsx +++ b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.tsx @@ -15,13 +15,14 @@ import type { Status } from 'src/components/StatusIcon/StatusIcon'; type ExtendedImageRegionStatus = 'unsaved' | ImageRegionStatus; interface Props { + disableRemoveButton?: boolean; onRemove: () => void; region: string; status: ExtendedImageRegionStatus; } export const ImageRegionRow = (props: Props) => { - const { onRemove, region, status } = props; + const { disableRemoveButton, onRemove, region, status } = props; const { data: regions } = useRegionsQuery(); @@ -40,6 +41,7 @@ export const ImageRegionRow = (props: Props) => { /> diff --git a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx index f50c82a36aa..fe6074b371f 100644 --- a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx +++ b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx @@ -1,7 +1,7 @@ import { yupResolver } from '@hookform/resolvers/yup'; import { updateImageRegionsSchema } from '@linode/validation'; import { useSnackbar } from 'notistack'; -import React, { useState } from 'react'; +import React from 'react'; import { useForm } from 'react-hook-form'; import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel'; @@ -32,8 +32,6 @@ export const ManageImageRegionsForm = (props: Props) => { const { data: regions } = useRegionsQuery(); const { mutateAsync } = useUpdateImageRegionsMutation(image?.id ?? ''); - const [selectedRegions, setSelectedRegions] = useState([]); - const { formState: { errors, isDirty, isSubmitting }, handleSubmit, @@ -50,9 +48,11 @@ export const ManageImageRegionsForm = (props: Props) => { try { await mutateAsync(data); - enqueueSnackbar('Image regions successfully updated.', { + enqueueSnackbar(`${image?.label}'s regions successfully updated.`, { variant: 'success', }); + + onClose(); } catch (errors) { for (const error of errors) { if (error.field) { @@ -79,22 +79,20 @@ export const ManageImageRegionsForm = (props: Props) => { for details on managing your Linux system's disk space. { - setValue('regions', [...values.regions, ...selectedRegions], { + onChange={(regionIds) => + setValue('regions', regionIds, { shouldDirty: true, shouldValidate: true, - }); - setSelectedRegions([]); - }} - regions={(regions ?? []).filter( - (r) => !values.regions.includes(r.id) && r.site_type === 'core' - )} + }) + } currentCapability={undefined} errorText={errors.regions?.message} + isClearable label="Add Regions" - onChange={setSelectedRegions} placeholder="Select Regions" - selectedIds={selectedRegions} + regions={regions?.filter((r) => r.site_type === 'core') ?? []} + renderTags={() => null} + selectedIds={values.regions} /> Image will be available in these regions ({values.regions.length}) @@ -127,6 +125,7 @@ export const ManageImageRegionsForm = (props: Props) => { (regionItem) => regionItem.region === regionId )?.status ?? 'unsaved' } + disableRemoveButton={values.regions.length <= 1} key={regionId} region={regionId} /> From 80efb4a76bece7b33cdd6e8174dc74d7e1233e9a Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Thu, 11 Jul 2024 12:47:33 -0400 Subject: [PATCH 2/6] Added changeset: Update Manage Image Regions Drawer based on UX feedback --- .../.changeset/pr-10674-upcoming-features-1720716452894.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 packages/manager/.changeset/pr-10674-upcoming-features-1720716452894.md diff --git a/packages/manager/.changeset/pr-10674-upcoming-features-1720716452894.md b/packages/manager/.changeset/pr-10674-upcoming-features-1720716452894.md new file mode 100644 index 00000000000..540c9b32b45 --- /dev/null +++ b/packages/manager/.changeset/pr-10674-upcoming-features-1720716452894.md @@ -0,0 +1,5 @@ +--- +"@linode/manager": Upcoming Features +--- + +Update Manage Image Regions Drawer based on UX feedback ([#10674](https://github.com/linode/manager/pull/10674)) From 4822e243a05e0fea7385d493d8f2140266cf4dec Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Thu, 11 Jul 2024 13:04:44 -0400 Subject: [PATCH 3/6] fix obj regression --- .../ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx | 3 +-- .../AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx index fe6074b371f..8487c5b87d3 100644 --- a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx +++ b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx @@ -87,9 +87,8 @@ export const ManageImageRegionsForm = (props: Props) => { } currentCapability={undefined} errorText={errors.regions?.message} - isClearable label="Add Regions" - placeholder="Select Regions" + placeholder="Select regions or type to search" regions={regions?.filter((r) => r.site_type === 'core') ?? []} renderTags={() => null} selectedIds={values.regions} diff --git a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx index f145eaee71d..210d0b62cd5 100644 --- a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx +++ b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx @@ -29,13 +29,15 @@ export const AccessKeyRegions = (props: Props) => { return ( 0 ? ' ' : 'Select regions or type to search' + } currentCapability="Object Storage" disabled={disabled} errorText={errorText} isClearable={false} label="Regions" - placeholder="Select Regions or type to search" + onChange={onChange} regions={regions ?? []} required={required} selectedIds={selectedRegion} From 2abbd4965f581e536dcd8e94b65fb3b270d30b9d Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Thu, 11 Jul 2024 13:33:18 -0400 Subject: [PATCH 4/6] update tests to reflect new UX --- .../core/images/manage-image-regions.spec.ts | 36 ++++++++++--------- .../ManageImageRegionsForm.test.tsx | 3 -- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/packages/manager/cypress/e2e/core/images/manage-image-regions.spec.ts b/packages/manager/cypress/e2e/core/images/manage-image-regions.spec.ts index 663125cd190..c09ccfe63fa 100644 --- a/packages/manager/cypress/e2e/core/images/manage-image-regions.spec.ts +++ b/packages/manager/cypress/e2e/core/images/manage-image-regions.spec.ts @@ -139,7 +139,7 @@ describe('Manage Image Regions', () => { // mock the updated paginated response mockGetCustomImages([updatedImage]); - // Click outside of the Region Multi-Select to commit the selection to the list + // Click outside of the Region Multi-Select to close the popover ui.drawer .findByTitle(`Manage Regions for ${image.label}`) .click() @@ -170,7 +170,7 @@ describe('Manage Image Regions', () => { // Verify the image isn't shown in the list after being removed cy.findByText(region1.label).should('not.exist'); - // Verify the count is now 2 + // Verify the count is now 3 cy.findByText('Image will be available in these regions (3)').should( 'be.visible' ); @@ -181,21 +181,9 @@ describe('Manage Image Regions', () => { .should('be.visible') .should('be.enabled') .click(); - - // "Unsaved" regions should transition to "pending replication" because - // they are now returned by the API - cy.findAllByText('pending replication').should('be.visible'); - - // The save button should become disabled now that changes have been saved - ui.button.findByTitle('Save').should('be.disabled'); - - // The save button should become disabled now that changes have been saved - ui.button.findByTitle('Save').should('be.disabled'); - - cy.findByLabelText('Close drawer').click(); }); - ui.toast.assertMessage('Image regions successfully updated.'); + ui.toast.assertMessage(`${image.label}'s regions successfully updated.`); cy.findByText(image.label) .closest('tr') @@ -206,8 +194,24 @@ describe('Manage Image Regions', () => { // Verify the first region is rendered cy.findByText(region2.label + ',').should('be.visible'); - // Verify the regions count is now "+2" cy.findByText('+2').should('be.visible').should('be.enabled'); + + // Verify the regions count is now "+2" and open the drawer + cy.findByText('+2').should('be.visible').should('be.enabled').click(); + }); + + ui.drawer + .findByTitle(`Manage Regions for ${image.label}`) + .click() + .within(() => { + // "Unsaved" regions should transition to "pending replication" because + // they are now returned by the API + cy.findAllByText('pending replication').should('be.visible'); + + // The save button should be disabled + ui.button.findByTitle('Save').should('be.disabled'); + + cy.findByLabelText('Close drawer').click(); }); }); }); diff --git a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.test.tsx b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.test.tsx index c3623e4d789..1a2478f355b 100644 --- a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.test.tsx +++ b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.test.tsx @@ -96,9 +96,6 @@ describe('ManageImageRegionsDrawer', () => { // Select new region await userEvent.click(await findByText('us-west', { exact: false })); - // Close the Region Multi-Select to that selections are committed to the list - await userEvent.type(regionSelect, '{escape}'); - expect(getByText('Place, CA')).toBeVisible(); expect(getByText('unsaved')).toBeVisible(); From 6ce79c18d1bdafd9d96ef509d6860d6722f37882 Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Mon, 15 Jul 2024 11:58:34 -0400 Subject: [PATCH 5/6] add fallback --- .../ImageRegions/ManageImageRegionsForm.tsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx index 8487c5b87d3..03f9fdb71a3 100644 --- a/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx +++ b/packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx @@ -48,9 +48,12 @@ export const ManageImageRegionsForm = (props: Props) => { try { await mutateAsync(data); - enqueueSnackbar(`${image?.label}'s regions successfully updated.`, { - variant: 'success', - }); + enqueueSnackbar( + `${image?.label ?? 'Image'}'s regions successfully updated.`, + { + variant: 'success', + } + ); onClose(); } catch (errors) { From 6df361f0a399d5b0aa36d6c29190614c05424f4b Mon Sep 17 00:00:00 2001 From: Banks Nussman Date: Mon, 15 Jul 2024 13:15:52 -0400 Subject: [PATCH 6/6] fix placeholder behvaior --- packages/manager/src/components/Autocomplete/Autocomplete.tsx | 2 +- .../AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/manager/src/components/Autocomplete/Autocomplete.tsx b/packages/manager/src/components/Autocomplete/Autocomplete.tsx index 65f21b124e9..8fe72590532 100644 --- a/packages/manager/src/components/Autocomplete/Autocomplete.tsx +++ b/packages/manager/src/components/Autocomplete/Autocomplete.tsx @@ -108,7 +108,7 @@ export const Autocomplete = < label={label} loading={loading} noMarginTop={noMarginTop} - placeholder={placeholder || 'Select an option'} + placeholder={placeholder ?? 'Select an option'} required={textFieldProps?.InputProps?.required} tooltipText={textFieldProps?.tooltipText} {...params} diff --git a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx index 210d0b62cd5..972caeb5611 100644 --- a/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx +++ b/packages/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx @@ -30,7 +30,7 @@ export const AccessKeyRegions = (props: Props) => { return ( 0 ? ' ' : 'Select regions or type to search' + selectedRegion.length > 0 ? '' : 'Select regions or type to search' } currentCapability="Object Storage" disabled={disabled}