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.

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

fix: [M3-8588] - Fix value selection issue in ImageSelect #11007

Merged
merged 8 commits into from
Sep 27, 2024
Merged

fix: [M3-8588] - Fix value selection issue in ImageSelect #11007

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ff2efe2
fix: [M3-8588] - Fix value selection issue in ImageSelect
carrillo-erik Sep 25, 2024
410c96a
Add changeset
carrillo-erik Sep 25, 2024
313f797
Update packages/manager/.changeset/pr-11007-fixed-1727295720802.md
carrillo-erik Sep 26, 2024
be3bd07
Update validation schema to account for clearing the value in ImageSe…
carrillo-erik Sep 26, 2024
ee99298
Merge branch 'develop' of https://github.com/linode/manager into fix/…
carrillo-erik Sep 26, 2024
52fcf47
Merge branch 'fix/M3-8588' of https://github.com/carrillo-erik/manage…
carrillo-erik Sep 26, 2024
f71ec73
Add changeset for validation pacakage
carrillo-erik Sep 26, 2024
e027133
Update CreateLinodeDiskFromImageSchema based on PR feedback
carrillo-erik Sep 27, 2024
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
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-11007-fixed-1727295720802.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Fixed
---

Fix value selection issue in ImageSelect ([#11007](https://github.com/linode/manager/pull/11007))
carrillo-erik marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 5 additions & 6 deletions packages/manager/src/features/Images/ImageSelect.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could use ImageSelectv2 here instead. This ImageSelect has some serious type safety issues and hackyness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai I agree with your concerns about the type safety issues and the hackyness feeling. I gave ImageSelectv2 a try and it appears that with some minor changes it should work within the CreateDiskDrawer.

The one issue I noticed is the way the options are grouped. In the current ImageSelect, one the three categories is the recommended 64-bit distributions. In the ImageSelectv2, they are grouped by vendor. Would require some conditional logic to determine with grouping function to use.

ImageSelect
ImageSelect
ImageSelectv2
ImageSelectv2

Copy link
Member

Choose a reason for hiding this comment

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

Ah good point. That would require some UX / product consideration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai To keep the work within the scope of this ticket, I'll create a separate ticket for UX to consider the grouping logic and see if it would make sense to further replace the original ImageSelect with the ImageSelectv2 component. Could please review and approve this for now. Thanks.

Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ export const ImageSelect = (props: MultiProps | Props) => {
}}
>
<Autocomplete
onChange={(event, value) => {
onChange={(_, value) => {
if (isMulti && Array.isArray(value)) {
onSelect((value ?? []) as Image[]);
} else if (!isMulti) {
onSelect(value as Image);
onSelect((value as Image) ?? null);
}
}}
textFieldProps={{
Expand All @@ -80,8 +80,9 @@ export const ImageSelect = (props: MultiProps | Props) => {
}}
value={
isMulti
? options.filter((o) => value?.includes(o.id)) ?? []
: options.find((o) => o.id === value) ?? null
? options.filter((option) => value?.includes(option.id)) ?? []
: options.find((option) => option.id === value) ??
(value ? null : undefined)
}
disableCloseOnSelect={false}
disableSelectAll
Expand All @@ -97,5 +98,3 @@ export const ImageSelect = (props: MultiProps | Props) => {
</Box>
);
};

export default ImageSelect;
3 changes: 2 additions & 1 deletion packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/ImageAndPassword.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ interface Props {
authorizedUsers: string[];
imageFieldError?: string;
linodeId: number;
onImageChange: (selected: Image) => void;
onImageChange: (image: Image) => void;
onPasswordChange: (password: string) => void;
password: string;
passwordError?: string;
Expand All @@ -38,6 +38,7 @@ export const ImageAndPassword = (props: Props) => {
const { data: profile } = useProfile();

const { data: imagesData, error: imagesError } = useAllImagesQuery();

const _imagesError = imagesError
? getAPIErrorOrDefault(imagesError, 'Unable to load Images')[0]?.reason
: undefined;
Expand Down
9 changes: 5 additions & 4 deletions packages/manager/src/features/Linodes/LinodesDetail/LinodeStorage/CreateDiskDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Disk, Linode } from '@linode/api-v4/lib/linodes';
import {
CreateLinodeDiskFromImageSchema,
CreateLinodeDiskSchema,
Expand All @@ -12,7 +11,7 @@ import { Autocomplete } from 'src/components/Autocomplete/Autocomplete';
import { Drawer } from 'src/components/Drawer';
import { FormHelperText } from 'src/components/FormHelperText';
import { InputAdornment } from 'src/components/InputAdornment';
import { Mode, ModeSelect } from 'src/components/ModeSelect/ModeSelect';
import { ModeSelect } from 'src/components/ModeSelect/ModeSelect';
import { Notice } from 'src/components/Notice/Notice';
import { TextField } from 'src/components/TextField';
import { useEventsPollingActions } from 'src/queries/events/events';
Expand All @@ -26,6 +25,8 @@ import { handleAPIErrors } from 'src/utilities/formikErrorUtils';
import { ImageAndPassword } from '../LinodeSettings/ImageAndPassword';

import type { Image } from '@linode/api-v4';
import type { Disk, Linode } from '@linode/api-v4/lib/linodes';
import type { Mode } from 'src/components/ModeSelect/ModeSelect';

type FileSystem = 'ext3' | 'ext4' | 'initrd' | 'raw' | 'swap';

Expand Down Expand Up @@ -172,8 +173,8 @@ export const CreateDiskDrawer = (props: Props) => {
imageFieldError={
formik.touched.image ? formik.errors.image : undefined
}
onImageChange={(selected: Image) =>
formik.setFieldValue('image', selected?.id ?? null)
onImageChange={(image: Image) =>
formik.setFieldValue('image', image?.id ?? null)
}
onPasswordChange={(root_pass: string) =>
formik.setFieldValue('root_pass', root_pass)
Expand Down
4 changes: 1 addition & 3 deletions packages/manager/src/features/StackScripts/StackScriptForm/StackScriptForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { InputAdornment } from 'src/components/InputAdornment';
import { Paper } from 'src/components/Paper';
import { TextField } from 'src/components/TextField';
import { Typography } from 'src/components/Typography';
import ImageSelect from 'src/features/Images/ImageSelect';
import { ImageSelect } from 'src/features/Images/ImageSelect';
import { getAPIErrorFor } from 'src/utilities/getAPIErrorFor';

import {
Expand Down Expand Up @@ -174,5 +174,3 @@ export const StackScriptForm = React.memo((props: Props) => {
</Paper>
);
});

export default React.memo(StackScriptForm);
Loading