-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
[react] Wrap individual css output into their own cascade layer #240
base: master
Are you sure you want to change the base?
Conversation
Also, update the base style output to include pigment cascase layer order
If this issue is purely in Next.js, why do you make this change for Vite also? |
The original issue is for next.js but the solution here is generic that we were anyways planning to implement. This would have helped with better css precedence for base and variant styles. |
// // Bring each file in its own layer so that the order is maintained between css modules | ||
// // shared between layout.tsx and page.tsx. | ||
// // TODO: Do this in a way that keeps the source map correct | ||
// cssText = ` |
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 don't think we are solving the same issue. We discussed briefly with @Janpot, namely the layering for the base, overrides and variants styles should be there, but we should not remove this, because the conflict we had in the previous issue comes from defining two base styles (as we use the styled() call on a styled component):
// these are both styles that would be added in the base layer, no?
const Paper = styled(div)({});
const AppBar = styled(Paper)({});
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.
You can see that problem demonstrated here. (original reproduction with the csbci output of this PR)
Look at the menu, under home the button texts are center aligned, under about they are left aligned. If you remove the content of the Container
component on the home page, the menu items suddenly become left aligned.
This is what the "layer per file" fixed, it's essentially the problem described here (kudos @siriwatknp). It makes sure the orginal import order is preserved, even when a subset of them is being loaded again in a subsequent chunk.
Would we be able to also keep the per-file layers? They can be the outer layer. I also don't think this is necessarily going to be a Next.js only issue. I could see this happen in other CSS code splitting scenarios.
One immediate downside I see is increase in generated CSS size since each individual css output is wrapped in a layer.
Endusers might need to include a PostCSS plugin to merge layers,media queries and classNames.
I imagine it would compress quite well though, so it may not seem as big of a problem as we'd expect.
Also, update the base style output to include pigment cascade layer order.
This change wraps the CSS output into its own layers and then the base css style output determines the precedence of the layers.
Lets assume this to be kind of an RFC PR to discuss the usage and impact of Cascade layers.
One immediate downside I see is increase in generated CSS size since each individual css output is wrapped in a layer.
Endusers might need to include a PostCSS plugin to merge layers,media queries and classNames.
This fixes #199