refactor: [M3-8339] - Clean up and fix Linode Details after CDS 2.0 #10662
Conversation
bnussman-akamai
added
Clean Up
Design Tokens
bnussman-akamai
requested review from
hkhalil-akamai and
abailly-akamai
and removed request for
a team
| @@ -858,7 +858,7 @@ export const darkTheme: ThemeOptions = { | |||
| palette: { | |||
| background: { | |||
| default: customDarkModeOptions.bg.app, | |||
| paper: Color.Neutrals[100], | |||
| paper: Color.Neutrals[90], | |||
There was a problem hiding this comment.
bg.bgPaper is set to Color.Neutrals[90] so I think this should probably be set to the same thing.
bg.bgPaper should probably be phased out in the long run in favor of MUI's built in palette value theme.palette.background.paper
There was a problem hiding this comment.
We will clean some of that up in future theme layer updates. Ideally we don't want to use either directly anyway. In our perfect world we won't have any exportable theme constants.
| <StyledTypography variant="h3">IP Addresses</StyledTypography> | ||
| </Grid> | ||
| <StyledWrapperGrid> | ||
| <Hidden smDown> |
There was a problem hiding this comment.
I removed this Hidden because we should probably be allowing mobile users to access IP Transfer and IP Sharing
There was a problem hiding this comment.
This does lead to some wrapping at narrow widths but it doesn't affect usability:
There was a problem hiding this comment.
Updated this to switch over to an ActionMenu on small screens in a22ef07
bnussman-akamai
changed the title
|
Coverage Report: ✅ |
| > | ||
| <Stack> | ||
| <Box | ||
| bgcolor={(theme) => theme.palette.background.paper} |
There was a problem hiding this comment.
Why are we adding a background color to the headers in this section, is it to match what we had before?
There was a problem hiding this comment.
Yeah, exactly. The previous implementation was using theme.color.white, but since CDS was merged, the color changed causing these headers to have no visible background in dark mode. I updated this to use theme.palette.background.paper, which seems more fitting anyways.
We could probably use a Paper instead of a Box here, but I went with Box to keep things more "raw"
There was a problem hiding this comment.
I don't understand what keep things more "raw"means but using the right component is always the better option VS a one time style, especially as the theme will go through more changes in the near future
There was a problem hiding this comment.
Good point. I initially hesitated to use Paper because I was worried that its built-in padding would cause problems, but it doesn't matter because I'm specifying the padding manually anyway
Addressed in 8730aae
bnussman-akamai
changed the title
| > | ||
| <Stack> | ||
| <Box | ||
| bgcolor={(theme) => theme.palette.background.paper} |
There was a problem hiding this comment.
I don't understand what keep things more "raw"means but using the right component is always the better option VS a one time style, especially as the theme will go through more changes in the near future
packages/manager/src/features/Linodes/LinodesDetail/LinodeStorage/LinodeVolumes.tsx
Outdated
Show resolved
Hide resolved
| @@ -858,7 +858,7 @@ export const darkTheme: ThemeOptions = { | |||
| palette: { | |||
| background: { | |||
| default: customDarkModeOptions.bg.app, | |||
| paper: Color.Neutrals[100], | |||
| paper: Color.Neutrals[90], | |||
There was a problem hiding this comment.
We will clean some of that up in future theme layer updates. Ideally we don't want to use either directly anyway. In our perfect world we won't have any exportable theme constants.
packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeIPAddresses.tsx
Show resolved
Hide resolved
Description 📝
styledcomponents 🗑️Grid🗑️AddNewLinkwithButton🔘Preview 📷
How to test 🧪
As an Author I have considered 🤔