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

[enhancement] Reorder the css generation order for styled calls #56

Merged
merged 1 commit into from
May 8, 2024

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented May 8, 2024

to match with @mui/system

Fixes #55

@brijeshb42 brijeshb42 requested a review from siriwatknp May 8, 2024 08:44
@siriwatknp siriwatknp added the bug 🐛 Something doesn't work label May 8, 2024
@brijeshb42 brijeshb42 added enhancement This is not a bug, nor a new feature and removed enhancement This is not a bug, nor a new feature labels May 8, 2024
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks a lot

@siriwatknp
Copy link
Member

Tested, it works like a charm!

@brijeshb42 brijeshb42 merged commit f8bcca7 into mui:master May 8, 2024
11 checks passed
@brijeshb42 brijeshb42 deleted the override-precedence branch May 8, 2024 10:30
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I can understand how both of these orders would make sense. Personally, I would go with the previous implementation, where basic styles override basic styles, and variants override variants - we weren't able to do this previously with emotion, as the styles were defined via callback and people could have used props there. I remember for example this issue: mui/material-ui#29529 where the order was not really intuitive.

I will try to get more insights into what would be more intuitive solution.

* 3. Variants declared in styled call
* 2. CSS declared in theme object's styledOverrides
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number of the list are wrong.

@brijeshb42
Copy link
Contributor Author

brijeshb42 commented May 8, 2024

Yeah. The only reason to change this is to maintain compatibility with system. I had also discussed this order way back during the early implementations and why that made sense.

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

Successfully merging this pull request may close these issues.

[styled] theme style overrides is overridden by variants
3 participants