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

[theme] Fix pseudo class overridden in variants #27847

Merged
merged 3 commits into from
Aug 21, 2021

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Aug 19, 2021

Closes #26944

Problem:
You can see that the styles of pseudo classes of variants are not merged correctly here: https://codesandbox.io/s/musing-darkness-2cd8d?file=/src/App.js:119-613

Solution: Following @oliviertassinari 's suggestion (#26944 (comment)), using an array to store variant styles rather than an object does the fix.

@@ -33,7 +33,7 @@ const getVariantStyles = (name, theme) => {

 const variantsResolver = (props, styles, theme, name) => {
   const { styleProps = {} } = props;
-  let variantsStyles = {};
+  const variantsStyles = [];
   const themeVariants = theme?.components?.[name]?.variants;
   if (themeVariants) {
     themeVariants.forEach((themeVariant) => {
@@ -44,7 +44,7 @@ const variantsResolver = (props, styles, theme, name) => {
         }
       });
       if (isMatch) {
-        variantsStyles = { ...variantsStyles, ...styles[propsToClassKey(themeVariant.props)] };
+        variantsStyles.push(styles[propsToClassKey(themeVariant.props)]);
       }
     });
   }

@hbjORbj hbjORbj changed the title [theme] Fix peudo class overridden in variants [theme] Fix pseudo class overridden in variants Aug 19, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 19, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 8a608a6

@hbjORbj hbjORbj added bug 🐛 Something doesn't work package: system Specific to @mui/system labels Aug 19, 2021
@mnajdova
Copy link
Member

mnajdova commented Aug 19, 2021

Perfect! Let’s add test for this use-case, we can create a simple component that mimic the behavior of the button.


We can use something like this in the test:

https://codesandbox.io/s/long-hill-tiup4?file=/src/App.js

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.

Perfect! I verified that the test indeed is failing on current next.

@hbjORbj hbjORbj merged commit a4e2ed8 into mui:next Aug 21, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 21, 2021

@hbjORbj Prefer this syntax:

Capture d’écran 2021-08-21 à 19 50 03

It allows closing the underlying issue automatically once the PR is merged.

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

Successfully merging this pull request may close these issues.

[theme] pseudo class overridden in variants
4 participants