-
Notifications
You must be signed in to change notification settings - Fork 367
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
change: [M3-8807] - Improve Linode Create VPC user experience #11188
change: [M3-8807] - Improve Linode Create VPC user experience #11188
Conversation
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 component is a bit crazy. It was a shared component used on Linode Create v1 and in the Linode Config dialog.
Now that Linode Create v1 is gone, we can clean this up because Linode Create v2 does not share any of this code.
Hopefully I don't break anything here in the process 😅
/** | ||
* A function that is called when a VPC is successfully created | ||
*/ | ||
onSuccess: (vpc: VPC) => void; |
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 find onSuccess
to be a but more understandable than handleSelectVPC
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.
That's fine.. our handlers naming conventions are really all over the place 😅
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.
The diff here is a bit hard to read but I essentially just deleted any test that tested the prop from="linodeCreate"
. The "from" prop doesn't need to exist anymore now that Linode Create doesn't use this component
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, thanks for the clarity improvements too ✅
No regressions when testing Linode creation with VPC
packages/manager/cypress/e2e/core/linodes/create-linode-with-vpc.spec.ts
Show resolved
Hide resolved
/** | ||
* A function that is called when a VPC is successfully created | ||
*/ | ||
onSuccess: (vpc: VPC) => void; |
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.
That's fine.. our handlers naming conventions are really all over the place 😅
@@ -135,12 +136,12 @@ export const useCreateVPC = (inputs: UseCreateVPCInputs) => { | |||
}; | |||
|
|||
try { | |||
const response = await createVPC(createVPCPayload); | |||
const vpc = await createVPC(createVPCPayload); |
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.
✅ confirmed updated unit and cypress test pass + test failure in cypress run looks unrelated
✅ verified that subnet gets preselected if VPC only has one subnet, for both creating a new VPC and choosing an already existing VPC
✅ verified subnet doesn't get preselected if VPC has 2+ subnets
✅ vpc configuration panel
Thanks Banks!! This is some really nice cleanup + added clarity! 🎉
(...on a side note, now that I'm looking at the VPC Create code again, I kinda wanna clean that up + switch it all to react-hook-form too... gonna try to get that in before VPC part 2 starts up!! 🤞🙏 )
Description 📝
Preview 📷
Screen.Recording.2024-10-29.at.8.40.03.PM.mov
Screen.Recording.2024-10-29.at.8.41.15.PM.mov
How to test 🧪
As an Author I have considered 🤔