-
Notifications
You must be signed in to change notification settings - Fork 364
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-7869] - Linode Create Refactor - Part 2 - Images and Distributions #10281
upcoming: [M3-7869] - Linode Create Refactor - Part 2 - Images and Distributions #10281
Conversation
const { field, fieldState } = useController<CreateLinodeRequest>({ | ||
name: 'type', | ||
}); | ||
|
||
const { data: regions } = useRegionsQuery(); | ||
const { data: types } = useAllTypes(); | ||
|
||
const regionId = watch('region'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out, this is very bad.
It resulted in the entire form re-rendering when the region changed. With this new flow, I'm going to focus on performance and fine-grain rendering.
Changing out this watch
call for a useWatch
makes it so that only this component subscribes to region
changing.
This is what makes react-hook-form
great. They build things in a way so that you have to be very intentional with what state updates you subscribe to.
<Tabs | ||
index={currentTabIndex} | ||
onChange={(index) => updateParams({ type: tabs[index] })} | ||
> | ||
<TabList> | ||
<Tab>Distributions</Tab> | ||
<Tab>Marketplace</Tab> | ||
<Tab>StackScripts</Tab> | ||
<Tab>Images</Tab> | ||
<Tab>Backups</Tab> | ||
<Tab>Clone Linode</Tab> | ||
</TabList> | ||
<TabPanels> | ||
<SafeTabPanel index={0}> | ||
<Distributions /> | ||
</SafeTabPanel> | ||
<SafeTabPanel index={1}>Marketplace</SafeTabPanel> | ||
<SafeTabPanel index={2}>StackScripts</SafeTabPanel> | ||
<SafeTabPanel index={3}> | ||
<Images /> | ||
</SafeTabPanel> | ||
<SafeTabPanel index={4}>Bckups</SafeTabPanel> | ||
<SafeTabPanel index={5}>Clone Linode</SafeTabPanel> | ||
</TabPanels> | ||
</Tabs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated moving this chunk into another component to keep this root file super clean but I decided keeping it here helps your brain match up the code with the UI.
Curious to hear what others think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it this way, the file remains pretty short and readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, it's fine to keep it here.
Coverage Report: ✅ |
@@ -1,9 +1,8 @@ | |||
import CssBaseline from '@mui/material/CssBaseline'; | |||
import 'font-logos/assets/font-logos.css'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this import to DistributionIcon.tsx
. Moving this import down the tree makes it so font-logos.css
isn't loaded until it is needed. 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good in testing and code is clean and readable. Left a few minor comments. Great to see this work proceed @bnussman-akamai!
const value = images?.find((i) => i.id === props.value); | ||
|
||
return ( | ||
<Autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I should do that for the 1:1 parity with the old component.
I left it simple for now because I prefer the clean look 😖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping it vs. not keeping it might be worth raising during Cafe this week
// @todo this is temporary API error handling. We will develop a more | ||
// robust helper that can convert API errors to react-hook-form errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for removing this todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because this whole entire flow could have "@todo"s everywhere. I don't wanna go crazy with them. I also think this error handling might work as is.
<Tabs | ||
index={currentTabIndex} | ||
onChange={(index) => updateParams({ type: tabs[index] })} | ||
> | ||
<TabList> | ||
<Tab>Distributions</Tab> | ||
<Tab>Marketplace</Tab> | ||
<Tab>StackScripts</Tab> | ||
<Tab>Images</Tab> | ||
<Tab>Backups</Tab> | ||
<Tab>Clone Linode</Tab> | ||
</TabList> | ||
<TabPanels> | ||
<SafeTabPanel index={0}> | ||
<Distributions /> | ||
</SafeTabPanel> | ||
<SafeTabPanel index={1}>Marketplace</SafeTabPanel> | ||
<SafeTabPanel index={2}>StackScripts</SafeTabPanel> | ||
<SafeTabPanel index={3}> | ||
<Images /> | ||
</SafeTabPanel> | ||
<SafeTabPanel index={4}>Bckups</SafeTabPanel> | ||
<SafeTabPanel index={5}>Clone Linode</SafeTabPanel> | ||
</TabPanels> | ||
</Tabs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it this way, the file remains pretty short and readable.
<Tab>Distributions</Tab> | ||
<Tab>Marketplace</Tab> | ||
<Tab>StackScripts</Tab> | ||
<Tab>Images</Tab> | ||
<Tab>Backups</Tab> | ||
<Tab>Clone Linode</Tab> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be better expressed as a tabs.map
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, but because this page is likely not going to change much, I think keeping it static is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is with currentTabIndex
going out of sync, but as you said with these not changing often its unlikely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<i | ||
className={className} | ||
data-testid="distro-icon" | ||
style={{ fontSize: size ?? '1.8em' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the icon size to be relative to the parent? I am not saying we don't, i am frankly wondering cause sometimes it can cause unwanted side effects. Since we can override the size am wondering about using px instead for the default. No big deal either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this component in 5981137
It will now just inherit the fontSize and have no default.
import * as React from 'react'; | ||
import { OptionProps } from 'react-select'; | ||
import { makeStyles } from 'tss-react/mui'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfectionist/sort-imports
🤷
@@ -137,6 +137,9 @@ const isMemo = (prevProps: ImageSelectProps, nextProps: ImageSelectProps) => { | |||
); | |||
}; | |||
|
|||
/** | |||
* @deprecated Start using ImageSelectv2 when possible | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extending the scope is fine, but let's keep going with our recommanded practices? This one definitely warrants a unit test and to be in Storybook since we do that for our custom selects (LinodeSelect, RegionSelect etc). A bit of of overhead but worth it IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some testing in 1bf9aa5
* for the Images Select. | ||
* | ||
* Please use API filtering (getAPIFilterForImageSelect) when possible! | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the original behavior here (with legacy ImageSelect
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImageSelect
just had intense client side filtering / sorting. This new implementation should be much more simple!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right right just trying to understand if anything is changing to the logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are changes to logic. packages/manager/src/components/ImageSelect/ImageSelect.tsx
does a lot of crazy stuff and this new component likely doesn't work exactly like the old one.
For example, there is an API bug that is breaking the ordering on this new component. I filed an ARB for it already!
This is something we can iterate on as we need other functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnussman-akamai Does the bug you're referring to cover the reason why, for example, openSUSE images get listed under separate headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly that! @dwiley-akamai
<Tabs | ||
index={currentTabIndex} | ||
onChange={(index) => updateParams({ type: tabs[index] })} | ||
> | ||
<TabList> | ||
<Tab>Distributions</Tab> | ||
<Tab>Marketplace</Tab> | ||
<Tab>StackScripts</Tab> | ||
<Tab>Images</Tab> | ||
<Tab>Backups</Tab> | ||
<Tab>Clone Linode</Tab> | ||
</TabList> | ||
<TabPanels> | ||
<SafeTabPanel index={0}> | ||
<Distributions /> | ||
</SafeTabPanel> | ||
<SafeTabPanel index={1}>Marketplace</SafeTabPanel> | ||
<SafeTabPanel index={2}>StackScripts</SafeTabPanel> | ||
<SafeTabPanel index={3}> | ||
<Images /> | ||
</SafeTabPanel> | ||
<SafeTabPanel index={4}>Bckups</SafeTabPanel> | ||
<SafeTabPanel index={5}>Clone Linode</SafeTabPanel> | ||
</TabPanels> | ||
</Tabs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, it's fine to keep it here.
export const useLinodeCreateQueryParams = () => { | ||
const history = useHistory(); | ||
|
||
const rawParams = getQueryParamsFromQueryString(history.location.search); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you are not using useParams
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to 1:1 parity with the existing create flow and it only uses query params. I think useParams
is only for router path params
|
||
const updateParams = (params: Partial<LinodeCreateQueryParams>) => { | ||
const newParams = new URLSearchParams({ ...rawParams, ...params }); | ||
history.replace({ search: newParams.toString() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
history.replace
will prevent the browser from remember the history, therefore not allowing the browser back and forward buttons. This introduces regressions with the current behavior. history.push
is probably what you want here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Thanks! 4a3e081
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will circle back to wrap up my review tomorrow morning!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: can we do "V2" in the directory and file names instead of "v2"? Just makes it easier to notice when looking quickly
* for the Images Select. | ||
* | ||
* Please use API filtering (getAPIFilterForImageSelect) when possible! | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnussman-akamai Does the bug you're referring to cover the reason why, for example, openSUSE images get listed under separate headers?
@@ -47,7 +48,7 @@ export const Plan = () => { | |||
selectedId={field.value} | |||
selectedRegionID={regionId} | |||
showTransfer | |||
types={types?.map(extendType) ?? []} | |||
types={types?.map(extendType) ?? []} // @todo don't extend type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a notation like @TODO Linode Create Refactor
to make it easy to track these down during this refactoring effort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might leave it as a basic todo because that change isn't within the scope of this project.
const value = images?.find((i) => i.id === props.value); | ||
|
||
return ( | ||
<Autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping it vs. not keeping it might be worth raising during Cafe this week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the changes!
PR is good to go from my end, for future ones I'd still love to see out of scope commits (such as the new ImageSelect) decoupled in a separate PR - especially since it seems like this is prompting the most questions on this PR and diverting the attention from the create flow refactor. My two cents!
Description 📝
Part 2 of the Linode Create refactor effort 🎉
Changes 🔄
ImageSelectv2
Paper
baked into it 🙄LandingHeader
to match the existing Linode Createwatch
touseWatch
to fix other components re-rendering when changingregion
Preview 📷
Putting a before and after so you can how the refactor is progressing 🤩
How to test 🧪
As an Author I have considered 🤔