upcoming: [M3-7928] - Linode Create Refactor - Summary - Part 8 #10334
Conversation
bnussman-akamai
requested review from
hana-linode,
jaalah-akamai,
hkhalil-akamai and
abailly-akamai
and removed request for
a team
| <Stack | ||
| alignItems="center" | ||
| direction="row" | ||
| key={item.title} | ||
| spacing={1} | ||
| > | ||
| <Typography fontFamily={(theme) => theme.font.bold}> | ||
| {item.title} | ||
| </Typography> | ||
| {item.details && <Typography>{item.details}</Typography>} | ||
| </Stack> |
There was a problem hiding this comment.
I was in the fence about making this stuff into components. I think this file is simple enough to leave it like this, but I'm open to feedback
There was a problem hiding this comment.
This file handles the entire summary. I'm pretty happy with it but if anyone has better ideas for how to clean it up or improve it let me know
There was a problem hiding this comment.
Thanks for these thorough tests!
| { | ||
| item: { | ||
| details: `$${backupsPrice}/month`, | ||
| title: 'Backups', | ||
| }, | ||
| show: backupsEnabled, | ||
| }, |
There was a problem hiding this comment.
Not sure if this is an intentional change or not: in the old flow, backups pricing is show only after the user selects a region. Now, it displays as "$--.--/month"
There was a problem hiding this comment.
Good catch. I'll adjust to match the existing behavior.
|
Coverage Report: ✅ |
There was a problem hiding this comment.
@bnussman-akamai we're missing the placement group summary item
| }, | ||
| show: Boolean(firewallId), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
nit: i could see this in a Summary.data.ts file
| flexItem | ||
| orientation="vertical" | ||
| sx={{ borderWidth: 1, margin: 0 }} | ||
| /> |
There was a problem hiding this comment.
we're going for parity so it's fine to keep things as is, but i can see a case to use a Grid so we can have several columns on a medium bp? not a big deal either way, we can ask UX.
There was a problem hiding this comment.
I didn't go the grid route, but I improved wrapping so that is is as good as the existing behavior - c2161ab
| <Stack | ||
| alignItems="center" | ||
| direction="row" | ||
| key={item.title} | ||
| spacing={1} | ||
| > | ||
| <Typography fontFamily={(theme) => theme.font.bold}> | ||
| {item.title} | ||
| </Typography> | ||
| {item.details && <Typography>{item.details}</Typography>} | ||
| </Stack> |
…de#10334) * inital summary * more work and unit tests * Added changeset: Linode Create Refactor - Summary - Part 8 * only show backups if a type is selected * add placement group to summary * fix summary wrapping --------- Co-authored-by: Banks Nussman <banks@nussman.us>
Description 📝
Preview 📷
How to test 🧪
Prerequisites
Linode Create v2flag 🎏Verification steps
As an Author I have considered 🤔