-
Notifications
You must be signed in to change notification settings - Fork 373
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
refactor: [M3-9591] - Cleanup token organization and paths #11867
Conversation
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 recommending these, I think this will bring clarity around the tokens as well as simplicity.
component: Component, | ||
font: Font, | ||
spacing: Spacing, | ||
}, |
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 actually forget why these are needed here apart from the base light.ts
file?
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 looks very much like a @bnussman-akamai thing -
// This is a hack to create breakpoints outside of the theme itself.
// This will likely have to change at some point 😖
it's unclear from the comment what the purpose is
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 removed the token declarations from here , assuming they are not needed. Maybe Banks can chime in on this
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.
If I remember correctly, @mui/material
removed (or doesn't have?) the ability to create breakpoints outside of the theme, which we rely on somewhat heavily in light.ts
and dark.ts
so I had to use createTheme
as a hack here to generate the breakpoints.
light.ts
and dark.ts
import breakpoints
and use it construct the theme.
Look at this diff for context: https://github.com/linode/manager/pull/11081/files#diff-273e3bd36fcb185d72c1982521d282dd0ea1d90bcc453b1706125a18438aa70e
Ideally, we'd define the breakpoint inside of lights.ts and reference breakpoints
by using a function within the theme
MuiTab: {
styleOverrides: {
root: ({ theme }) => ({
[theme.breakpoints.up('md')]: {
minWidth: 75,
},
}),
},
},
rather than what we do, which is:
MuiTab: {
styleOverrides: {
root: {
[breakpoints.up('md')]: {
minWidth: 75,
},
},
},
}
but we can't use the function notation in our theme because MUI's deepmerge
does not support the function syntax, which is what we use to merge our light and dark theme to create the dark theme. I tried this and it caused dark mode regressions.
In summary:
We must define our breakpoint values outside of the theme. If we try to define breakpoint values directly in the theme and reference them with the ({ theme }) => ({})
syntax, dark mode regressions occur because our theme merging does not support it. I had to use a hack to allow us to create breakpoints outside of the theme
Looking at this code now, maybe we can use the (new?) unstable_createBreakpoints
to get things back in a sane state?
This is what I meant by "This will likely have to change at some point 😖
"
This file would become:
import { unstable_createBreakpoints } from '@mui/material';
export const breakpoints = unstable_createBreakpoints({
values: {
lg: 1280,
md: 960,
sm: 600,
xl: 1920,
xs: 0,
},
});
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 a lot for this explanation @bnussman-akamai - this makes a lot more sense now. I will create a separate ticket to address this as I want to keep the scope reasonable for testing the refactor from this PR
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.
✅ Pulled this down and tokens are working as expected.
✅ Light & Dark themes are looking good
Cloud Manager UI test results🔺 1 failing test on test run #6 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/firewalls/migrate-linode-with-firewall.spec.ts" |
|
||
<Paper sx={(theme) => ({ backgroundColor: theme.semanticTokens.textColors.primary })}> | ||
<Paper sx={(theme) => ({ backgroundColor: theme.tokens.alias.Content.Text.Primary })}> |
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 changed this btw since semanticTokens
is not a thing
@@ -14,5 +13,4 @@ export const breakpoints = createTheme({ | |||
}, | |||
}, | |||
name: 'light', | |||
tokens: { chart: Chart, search: Search }, |
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.
So in the end I'm hoping this won't have negative consequences
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.
should be fine ✔️
Description 📝
This PR refactors our tokens so they are better organized and easier to use. It reduces our access to tokens through these 5 categories (for now!) and exposes all component tokes to the application.
The main goal is facilitates token discovery for DX. A better categorization streamlines IDE suggestions and help cognitive token discovery while developing.
Changes 🔄
Preview 📷
This PR is targeting naming conventions & DX. There should be no functional or visual regressions
How to test 🧪
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅