-
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-7872] - Linode Create Refactor - StackScripts #10367
upcoming: [M3-7872] - Linode Create Refactor - StackScripts #10367
Conversation
This reverts commit f56e4ad.
Coverage Report: ✅ |
If there is only one image to select from, in Prod we're pre-populating "Choose an Image" dropdown - do we want to maintain this? |
In Prod, the linode label is pre-populated with the selected stackscript name, is that something we want to keep? I know you had opinions about that in the past. |
|
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.
Great work on this so far! I agree with a fresh approach to the StackScripts components. Will continue my review next 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.
Noticing that the StackScripts Image selection lives in its own card in contrast to prod where it shares a card with the StackScript Selection
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 believe that was a regression, so sections will now be spaced out
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.
Super nice refactor! left some minor code and style comments to improve a few things, stick to conventions and consider visual regression/changes
if ( | ||
filteredOptions?.length === 1 && | ||
props.onChange && | ||
selectIfOnlyOneOption |
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.
Aren't filteredOptions?.length === 1
& selectIfOnlyOneOption
doing the same thing?
Do we need the selectIfOnlyOneOption
optional prop?
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 added the selectIfOnlyOneOption
prop so that the functionality is op-in. I don't think it should be the default behavior
<Table> | ||
<TableHead> | ||
<TableRow> | ||
<TableCell sx={{ width: 20 }}></TableCell> |
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 personally prefer the older table header rendering without an extra cell for the radio. Not a big deal either way, but that's a regression from previous layout, even if meant as an improvement.
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 did it this way to match our other selectable tables like Plan selection and Service Transfers. I'd be happy to rework this so that it is one single table cell if this is too much of a breaking change.
onOpenDetails={() => setSelectedStackScriptId(stackscript.id)} | ||
onSelect={() => field.onChange(stackscript.id)} | ||
stackscript={stackscript} | ||
/> |
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.
on mobile PROD we wrap the "show details" CTA which leaves more room for the title
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.
Yep. I think I'd have to rework this so that it is one single table cell if we want this behavior. The wrapping looked kind of wacky to me, so that's why I went the route of using 3 table cells. I'd be happy to re-work this back to a single table cell if you think that's better
...es/manager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/StackScriptSelectionRow.tsx
Show resolved
Hide resolved
<TableCell actionCell sx={{ minWidth: 120 }}> | ||
<InlineMenuAction actionText="Show Details" onClick={onOpenDetails} /> | ||
</TableCell> | ||
</TableRow> |
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.
As mentioned in other comments, typography is a a bit different here. It's not really pretty originally with the bolding so not sure what is best. A quick UX back and forth could be helpful to clean that up
image: null, | ||
stackscript_data: null, | ||
stackscript_id: null, | ||
})); |
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 👍
history.push({ search: newParams.toString() }); | ||
}; | ||
|
||
const params = { | ||
imageID: rawParams.imageID as string | undefined, |
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.
imageID: rawParams.imageID as string | undefined, | |
imageId: rawParams.imageId as string | undefined, |
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 debated that change. I did imageID
because that is how it is represented in the query params (which we can't change because we need to keep feature parity), but I can change this variable to imageId
if we'd prefer that. I think imageID
helps the reader understand that it matches the query param, but I'm also cool with strictly following our camel case rule.
history.push({ search: newParams.toString() }); | ||
}; | ||
|
||
const params = { | ||
imageID: rawParams.imageID as string | undefined, | ||
stackScriptID: rawParams.stackScriptID |
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.
same, would be nice to stick to our Id
convention here and below
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.
Tested:
✅ Selecting account and community stackscripts
✅ Viewing stack script details
✅ Actually creating the Linode from stackscript
✅ Code review 🧼
Really nice work on this. Approved pending some minor tweaks raised by @abailly-akamai.
…0367) * initial work * filter options based on stackscript compatibility * implement complex preselection logic * clean up a bit * implement details dialog * fix some query param logic * lots of behavior changes to match production * fix image select clear behavior and fix table waypoint console error * improve validation for when image is `null` * first unit tests * add lots of unit testing * add stackscript event handler * hook up validation packages for realtime validation * use default validation behavior * Revert "hook up validation packages for realtime validation" This reverts commit f56e4ad. * handle resets when switching tabs * add changesets * add some comments * bold label and pre-select if only one option * improve ux * fix unit tests * add comment --------- Co-authored-by: Banks Nussman <banks@nussman.us>
Description 📝
Note
Because of the amount of work involved to implement StackScripts on the new Linode Create flow, it will be broken up into a few PRs. You can consider this Part 1. There will be a part 2. Maybe a part 3.
This PR begins to implement the "StackScripts" section of the new Linode Create flow 🎉
Main features ➕
Features that will come in Part 2
Preview 📷
How to test 🧪
Prerequisites
Linode Create v2
feature flag using the local dev toolsVerification steps
As an Author I have considered 🤔