-
Notifications
You must be signed in to change notification settings - Fork 357
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-7927] - Linode Create Refactor - Part 6 - Add-ons #10319
upcoming: [M3-7927] - Linode Create Refactor - Part 6 - Add-ons #10319
Conversation
{isEdgeRegionSelected && ( | ||
<Notice | ||
text="Backups and Private IP are currently not available for Edge regions." | ||
variant="warning" | ||
/> | ||
)} |
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 are other notices that will need to be added here related to cloning. I will add those when I add cloning support down the road.
Coverage Report: ✅ |
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 all the doc improvements and tests. This is looking great 🚀
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.
Disabled flow for restricted user w/o Linode Create permissions, functional for unrestricted users ✅
Add-Ons section disabled when Edge region selected ✅
Code review ✅
packages/manager/src/features/Linodes/LinodeCreatev2/Addons/PrivateIP.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreatev2/Addons/PrivateIP.test.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreatev2/Addons/Backups.test.tsx
Outdated
Show resolved
Hide resolved
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 and super well tested 🎉
- Confirmed behavior of the form and payload ✅
- Confirmed a
null
firewall_id
is taken by the API ✅
Left a couple non blocking comments
Also, reminder to add the default values to the distro and details label
</Stack> | ||
<Typography> | ||
{isAccountBackupsEnabled ? ( | ||
<React.Fragment> |
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.
are we still doing fragments this way rather than <>
?
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.
This was just copy paste from packages/manager/src/features/Linodes/LinodesCreate/AddonsPanel.tsx
. I'm open to using <>
if we want to prefer that
type, | ||
}); | ||
|
||
const selectedRegion = regions?.find((r) => r.id === regionId); |
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 am still finding this a bit flawed. Yes, this is relatively inexpensive but because of only storing the region ID in the form state instead of the whole region object we are probably running regions.find
5 or 6 times on the Linode create flow - maybe more. Not a big deal but not the cleanest
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, this is a great point. I was considering adding a useRegion
query to avoid needing to run a regions.find
everywhere, but this could introduce an extra fetch.
This might be worth it considering how often we call regions.find
throughout the app
Have any thoughts on that or other ideas?
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 it be possible/a good idea to break pattern and have the form state being the whole 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.
I don't think that would be great. I think it would make things a ton more complicated with regarding types and validation
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.
makes sense. then i think the best option would be a useMemo util so at least we're not recalculating every time?
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Description 📝
Adds the "Add-ons" section to the new Linode Create Flow 🎉
Changes 🔄
firewall_id
to be null in the Linode Create type because the API allows it ✅Preview 📷
How to test 🧪
Prerequisites
Verification steps
Private IP
checkbox and theBackups
checkbox ☑️As an Author I have considered 🤔