Skip to content
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.

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

upcoming: [M3-8340] - Update Manage Image Regions Drawer based on UX feedback #10674

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -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))
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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'
);
Expand All @@ -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')
Expand All @@ -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();
});
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Region } from '@linode/api-v4';
import CloseIcon from '@mui/icons-material/Close';
import React from 'react';

Expand All @@ -24,6 +23,7 @@ import type {
DisableRegionOption,
RegionMultiSelectProps,
} from './RegionSelect.types';
import type { Region } from '@linode/api-v4';

interface LabelComponentProps {
region: Region;
Expand Down Expand Up @@ -67,7 +67,7 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => {
selectedIds,
sortRegionOptions,
width,
onClose,
...rest
} = props;
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows us to override any prop of the Autocomplete.

Needed to make this change so I could override renderTags


const {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -155,7 +155,6 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => {
InputProps: {
required,
},
placeholder: selectedRegions.length > 0 ? '' : placeholder,
tooltipText: helperText,
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is a good default so I moved this special logic into AccessKeyRegions.tsx.

We want the placeholder to always show in the Manage Regions Drawer and this logic was messing that up

}}
autoHighlight
Expand All @@ -172,7 +171,7 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => {
options={regionOptions}
placeholder={placeholder ?? 'Select Regions'}
value={selectedRegions}
onClose={onClose}
{...rest}
/>
</StyledAutocompleteContainer>
{selectedRegions.length > 0 && SelectedRegionsList && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export interface RegionSelectProps<

export interface RegionMultiSelectProps
extends Omit<
EnhancedAutocompleteProps<Region, false>,
EnhancedAutocompleteProps<Region, true>,
Copy link
Member Author

Choose a reason for hiding this comment

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

The isMulti generic should be true for the RegionMultiSelectProps. This type was wrong.

'label' | 'onChange' | 'options'
> {
SelectedRegionsList?: React.ComponentType<{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,19 @@ describe('ImageRegionRow', () => {

expect(onRemove).toHaveBeenCalled();
});

it('disables the remove button if disableRemoveButton is true', async () => {
const { getByLabelText } = renderWithTheme(
<ImageRegionRow
disableRemoveButton
onRemove={vi.fn()}
region="us-east"
status="creating"
/>
);

const removeButton = getByLabelText('Remove us-east');

expect(removeButton).toBeDisabled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -40,6 +41,7 @@ export const ImageRegionRow = (props: Props) => {
/>
<IconButton
aria-label={`Remove ${region}`}
disabled={disableRemoveButton}
onClick={onRemove}
sx={{ p: 0.5 }}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -32,8 +32,6 @@ export const ManageImageRegionsForm = (props: Props) => {
const { data: regions } = useRegionsQuery();
const { mutateAsync } = useUpdateImageRegionsMutation(image?.id ?? '');

const [selectedRegions, setSelectedRegions] = useState<string[]>([]);

const {
formState: { errors, isDirty, isSubmitting },
handleSubmit,
Expand All @@ -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) {
Expand All @@ -79,22 +79,19 @@ export const ManageImageRegionsForm = (props: Props) => {
for details on managing your Linux system's disk space.
</Typography>
<RegionMultiSelect
onClose={() => {
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}
label="Add Regions"
onChange={setSelectedRegions}
placeholder="Select Regions"
selectedIds={selectedRegions}
placeholder="Select regions or type to search"
regions={regions?.filter((r) => r.site_type === 'core') ?? []}
renderTags={() => null}
selectedIds={values.regions}
/>
<Typography sx={{ mb: 1, mt: 2 }}>
Image will be available in these regions ({values.regions.length})
Expand Down Expand Up @@ -127,6 +124,7 @@ export const ManageImageRegionsForm = (props: Props) => {
(regionItem) => regionItem.region === regionId
)?.status ?? 'unsaved'
}
disableRemoveButton={values.regions.length <= 1}
key={regionId}
region={regionId}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@ export const AccessKeyRegions = (props: Props) => {

return (
<RegionMultiSelect
onChange={onChange}
placeholder={
selectedRegion.length > 0 ? ' ' : 'Select regions or type to search'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this space?

Suggested change
selectedRegion.length > 0 ? ' ' : 'Select regions or type to search'
selectedRegion.length > 0 ? '' : 'Select regions or type to search'

Copy link
Member Author

Choose a reason for hiding this comment

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

If I leave it as '' (with no space) the default placeholder shows, which isn't what we want

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, fixed with 6df361f

}
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}
Expand Down
Loading