-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[styled-engine] Add enableCssLayer
prop to StyledEngineProvider
#45428
[styled-engine] Add enableCssLayer
prop to StyledEngineProvider
#45428
Conversation
enableCssLayer
option to StyledEngineProviderenableCssLayer
prop to StyledEngineProvider
Netlify deploy previewhttps://deploy-preview-45428--material-ui.netlify.app/ @mui/joy: parsed: +0.06% , gzip: +0.11% Bundle size reportDetails of bundle changes (Toolpad) |
46e63b3
to
52ec40d
Compare
52ec40d
to
3409cb9
Compare
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 we add some tests? Looks like StyledEngineProvider
doesn't have any, but still worth it IMO.
Added the test. It's not that simple to test the generated CSS with It won't leak to user because it's not exported through barrel files. |
@siriwatknp can you add instructions on how to properly test this change to get a sense of how the final DX looks like? I'm testing in a Vite app and when I enable |
Added the instruction in the PR description. |
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 and it works as expected 👍 I'd love more eyes on this before approving, also on the testing strategy.
packages/mui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.js
Show resolved
Hide resolved
packages/mui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.test.js
Show resolved
Hide resolved
packages/mui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.js
Outdated
Show resolved
Hide resolved
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.
@siriwatknp LGTM, only one change from my side: https://github.com/mui/material-ui/pull/45428/files#r1983527460
TEST_INTERNALS_DO_NOT_USE.insert = (...args) => { | ||
rule = args[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.
Nit: We should move the override and delete of TEST_INTERNALS_DO_NOT_USE.insert
into beforeAll
and afterAll
calls
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.
Refactored.
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.
@siriwatknp there's still some left over from the refactor:
- line 33
delete
- The last two tests still have the
.insert
definition and thedelete
at the end
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.
🤦♂️ Opps, removed.
…terial-ui into feat/style-engine-csslayer
…/style-engine-csslayer
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.
LGTM ✅
@siriwatknp remember that if we need to cherry-pick to v6, the v6.x
label should be added as well
Cherry-pick PRs will be created targeting branches: v6.x |
First step for #44700.
enableCssLayer
prop (same prop name as the AppRouter integration)injectFirst
prop.The next step is to create a Tailwind v4 guide in a separate PR.
Try it out
enableCssLayer
to StyleEngineProvider componentThe reason to do this from Material UI side is to ensure that CSS layer order is configured correctly before Material UI components render.
Review recommendation
toggle "Hide whitespace"