refactor: [M3-8720] - One `ImageSelect` to rule them all #11058

abailly-akamai · 2024-10-08T02:09:40Z

Description 📝

This PR consolidates the three different ImageSelect we had in the codebase.

src/components/ImageSelectV2
src/components/ImageSelect (legacy)
src/features/Images/ImageSelect (used in StackscriptCreate for instance)

It unifies the experience for every feature where an image picker is featured and cleans up the codebase accordingly from unnecessary utils.

Changes 🔄

Consolidate ImageSelect to include treatment, styling and interfaces of all ImageSelect components, including the "All/Any" and multiple options
Fix sorting of groups and options
- Groups are sorted alphabetically, with the exception of private images ("My Images") being first
- Within groups, options are sorted by the created property (latests release first)
Get rid of console warnings for
- ImageSelect
- Dialog (unrelated but caught while testing)
e2e updates to look for the new Autocomplete
unit coverage updates and improvements
cleanup

Target release date 🗓️

Before (stackscript create, for instance)	After

How to test 🧪

Prerequisites

Verification steps

Verify the behavior and content of the following UIs while comparing to their production equivalent:

/linode/create (no changes)
/linodes/create?type=Images (no changes)
/linodes > Action menu > Rebuild
- rebuild from image (no changes)
- rebuild from stackscript (no changes)
linode/{id}/storage (linode must be powered down) > Add Disk > Create from Image (old grouping replaced)
stackscripts/create (old grouping replaced)

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-10-22T17:35:21Z

packages/manager/src/components/Dialog/Dialog.tsx

-    ...rest
-  } = props;
+export const Dialog = React.forwardRef(
+  (props: DialogProps, ref: React.Ref<HTMLDivElement>) => {


Unrelated to ImageSelect changes but why not fixing here 🧹

The only change here (the rest is indenting) is to forward to ref to avoid console errors such as in /linodes
Should silence quite a bunch of warnings.

abailly-akamai · 2024-10-22T17:38:31Z

packages/manager/src/components/ImageSelect/ImageOption.tsx

The diff looks weird here cause it looks like we are modifying the whole component, but it is because ImageOptionV2 is being renamed to ImageOption which was our legacy component (now deleted)

abailly-akamai · 2024-10-22T17:38:59Z

packages/manager/src/components/ImageSelect/ImageSelect.tsx

Same comment about the diff size

abailly-akamai · 2024-10-22T17:41:04Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeRebuild/LinodeRebuildDialog.tsx

-        <EnhancedSelect
-          onChange={(selected: Item<MODES>) => {
-            setMode(selected.value);
+        <Autocomplete


Taking this opportunity to replace this old deprecated Select with our new Autocomplete. one more down!

abailly-akamai · 2024-10-22T17:41:44Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeRebuild/RebuildFromImage.tsx

  const { data: regionsData, isLoading: isLoadingRegions } = useRegionsQuery();
-  const isLoading = isLoadingPreferences || isLoadingImages || isLoadingRegions;
+  const isLoading = isLoadingPreferences || isLoadingRegions;


We're not fetching images from the parent anymore

abailly-akamai · 2024-10-22T17:43:27Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/ImageAndPassword.tsx

+        errorText={imageFieldError}
+        onChange={onImageChange}
+        value={selectedImage}
+        variant="all"


I am requiring the variant property now, forces to make a decision since it affects API filtering

abailly-akamai · 2024-10-22T17:46:14Z

packages/manager/src/features/StackScripts/StackScriptCreate/StackScriptCreate.tsx

Changes to the stackscript create is due to removing fetching from the class component, which in turns allows us to fully get rid of the withImages HOC 🎉

github-actions · 2024-10-22T18:54:27Z

Coverage Report: ✅
Base Coverage: 87.05%
Current Coverage: 87.15%

bnussman-akamai

Looking solid on my initial pass!

I'm noticing that re-opening the ImageSelect does not scroll the currently selected Image into view like it use to

Screen.Recording.2024-10-22.at.4.50.43.PM.mov

abailly-akamai · 2024-10-23T12:49:27Z

@bnussman-akamai Yeah great catch, one has to be careful when overriding with renderGroup - turns out it wasn't needed since it was to remedy the deepMerge style deprecation which is now fixed 👍

bnussman-akamai · 2024-10-24T14:49:38Z

Heads up: I think there is a crash on the multi-select. I was seeing it on the StackScript create page

abailly-akamai · 2024-10-24T15:27:04Z

@bnussman-akamai yes thank you, rebase gone wrong - fixed (this will also fix the e2e)

bnussman-akamai

Looks great!

Didn't spot any regressions on the flows I checked, only improvements! The consistancy across all of the flows is great

So glad to finally have only one ImageSelect 🎉 🙏

hana-akamai · 2024-10-25T17:17:47Z

Nice work, no regressions noticed! 🧹 Looks like there's some merge conflicts to resolve

coliu-akamai

✅ confirmed tests pass
✅ confirmed flows listed in verification steps

this is awesome and so much nicer and consistent - thanks Alban!! 🚀 🎉

packages/manager/src/components/ImageSelect/ImageOption.test.tsx

packages/manager/src/features/Images/ImageSelect.tsx

cypress · 2024-10-29T19:13:17Z

Cloud Manager E2E Run #6754

Run Properties: Passed #6754 • 0cbb976196: refactor: [M3-8720] - One `ImageSelect` to rule them all (#11058)

Project	`Cloud Manager E2E`
Branch Review	`develop`
Run status	`Passed #6754`
Run duration	`25m 43s`
Commit	0cbb976196: refactor: [M3-8720] - One `ImageSelect` to rule them all (#11058)
Committer	`Alban Bailly`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`1`
Pending	`2`
Skipped	`0`
Passing	`445`
View all changes introduced in this branch ↗︎

abailly-akamai self-assigned this Oct 8, 2024

abailly-akamai force-pushed the M3-8720 branch 7 times, most recently from e883123 to 4585ec3 Compare October 17, 2024 15:54

abailly-akamai force-pushed the M3-8720 branch 3 times, most recently from 68d5676 to ba20dbb Compare October 22, 2024 13:55

abailly-akamai changed the title ~~refactor: [M3-8720] - Fully deprecate ImageSelect v1~~ refactor: [M3-8720] - One ImageSelect to rule them all Oct 22, 2024

abailly-akamai commented Oct 22, 2024

View reviewed changes

abailly-akamai marked this pull request as ready for review October 22, 2024 18:20

abailly-akamai requested review from a team as code owners October 22, 2024 18:20

abailly-akamai requested review from jdamore-linode, hana-akamai and cpathipa and removed request for a team October 22, 2024 18:20

bnussman-akamai reviewed Oct 22, 2024

View reviewed changes

abailly-akamai force-pushed the M3-8720 branch from 12ec809 to 6d57cf6 Compare October 23, 2024 12:25

abailly-akamai requested a review from bnussman-akamai October 23, 2024 12:49

bnussman-akamai approved these changes Oct 24, 2024

View reviewed changes

coliu-akamai approved these changes Oct 29, 2024

View reviewed changes

packages/manager/src/components/ImageSelect/ImageOption.test.tsx Show resolved Hide resolved

packages/manager/src/features/Images/ImageSelect.tsx Show resolved Hide resolved

abailly-akamai added 15 commits October 29, 2024 11:55

Initial commit - console errors and low hanging fruits

5b9b0c6

Renaming and moving things

9028d8f

Small clanup

14150c6

One ImageSelect to rule them all

f3c3eb5

One ImageSelect to rule them all

735d9ab

fixing e2e tests

0bd0892

save progress

c87d759

cleanup

3c3fbbc

cleanup

fff2b68

post rebase fix

657b89e

fix the console errors with autocomplete

b909532

sorting and ordering

53002b0

Added changeset: Consolidate ImageSelect components

2586afc

feedback @bnussman-akamai

ddf14fe

fix rebase gone wrong

c96670c

abailly-akamai force-pushed the M3-8720 branch from b8cdf6d to c96670c Compare October 29, 2024 16:03

feedback @coliu-akamai

aa41d60

abailly-akamai merged commit 0cbb976 into linode:develop Oct 29, 2024
23 checks passed

jaalah-akamai mentioned this pull request Nov 7, 2024

Release v1.132.0 - release → staging #11220

Merged

hana-akamai mentioned this pull request Nov 12, 2024

Release v1.132.0 - staging → master #11249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: [M3-8720] - One `ImageSelect` to rule them all #11058

refactor: [M3-8720] - One `ImageSelect` to rule them all #11058

abailly-akamai commented Oct 8, 2024 •

edited

Loading

abailly-akamai Oct 22, 2024

abailly-akamai Oct 22, 2024

abailly-akamai Oct 22, 2024

abailly-akamai Oct 22, 2024

abailly-akamai Oct 22, 2024

abailly-akamai Oct 22, 2024

abailly-akamai Oct 22, 2024

github-actions bot commented Oct 22, 2024 •

edited

Loading

bnussman-akamai left a comment

abailly-akamai commented Oct 23, 2024

bnussman-akamai commented Oct 24, 2024

abailly-akamai commented Oct 24, 2024

bnussman-akamai left a comment

hana-akamai commented Oct 25, 2024

coliu-akamai left a comment

cypress bot commented Oct 29, 2024

refactor: [M3-8720] - One ImageSelect to rule them all #11058

refactor: [M3-8720] - One ImageSelect to rule them all #11058

Conversation

abailly-akamai commented Oct 8, 2024 • edited Loading

Description 📝

Changes 🔄

Target release date 🗓️

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2024 • edited Loading

bnussman-akamai left a comment

Choose a reason for hiding this comment

abailly-akamai commented Oct 23, 2024

bnussman-akamai commented Oct 24, 2024

abailly-akamai commented Oct 24, 2024

bnussman-akamai left a comment

Choose a reason for hiding this comment

hana-akamai commented Oct 25, 2024

coliu-akamai left a comment

Choose a reason for hiding this comment

cypress bot commented Oct 29, 2024

refactor: [M3-8720] - One `ImageSelect` to rule them all #11058

refactor: [M3-8720] - One `ImageSelect` to rule them all #11058

abailly-akamai commented Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading