Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brijeshb42
Copy link
Contributor

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

Also, update the base style output to include pigment cascase layer order
@brijeshb42 brijeshb42 requested a review from a team September 17, 2024 09:02
@brijeshb42 brijeshb42 added bug 🐛 Something doesn't work nextjs labels Sep 17, 2024
@o-alexandrov
Copy link

If this issue is purely in Next.js, why do you make this change for Vite also?

@brijeshb42
Copy link
Contributor Author

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.
This as a side-effect also fixes the layout issue.

// // 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 = `
Copy link
Member

@mnajdova mnajdova Sep 23, 2024

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)({});

Copy link
Member

@Janpot Janpot Sep 23, 2024

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.

  • home:
    Screenshot 2024-09-23 at 10 28 56
  • about:
    Screenshot 2024-09-23 at 10 29 13

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work nextjs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component styles are overridden when rendered inside Next.js layout
4 participants