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

Fixes emotion compat, className overwriting more specific classes #33866

Closed
wants to merge 1 commit into from
Closed

Fixes emotion compat, className overwriting more specific classes #33866

wants to merge 1 commit into from

Conversation

garronej
Copy link
Contributor

@garronej garronej commented Aug 9, 2022

Hi,
This PR is related to #33205.

What is happening is that classNames overwrites the classes that applies to the root component.
I have implemented a fix for the Tab component. If you are fine with it I can generalize it.

The visual test case can be run with:

yarn test:regressions:dev

Then accessing: http://localhost:3000/regression-Table/EmotionCompat#no-dev

With this branch you'll get:
image

With mui/material-ui#master:
image

Best regard,

@mui-bot
Copy link

mui-bot commented Aug 9, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 9c4a028

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.

This will complicate things too much, it would mean that in each component we will need to merge the className prop at specific location. More over, how do we decide what "more specific classes" mean? I am saying this because there is no such thing as a more specific class, there are more specific selectors.


const slots = {
root: [
'root',
'className',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'className',

This will generate class name: MuiTab-className

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will generate class name: MuiTab-className
Sorry my implementation was naive, obviousy we dont want MuiTab-className.

@garronej
Copy link
Contributor Author

garronej commented Aug 15, 2022

This will complicate things too much

OK I understand why you don't see it as being worth fixing.

More over, how do we decide what "more specific classes" mean? I am saying this because there is no such thing as a more specific class, there are more specific selectors.

While I agree that I didn't phrase it correctly, I disagree on the fact that there is an ambiguity on what the expected behavior is.
If a user writes the following code, he will, for sure, expect the background to be green.

 <Tab
          icon={<PhoneIcon />}
          label="Background should be green"
          className={css({ backgroundColor: 'red' })}
          classes={{
            labelIcon: css({ backgroundColor: 'green' })
          }}
 />

Any classes specified in the classes props that apply to the root component is expected to take priority over the className. (Except for classes.root where it isn't clear but we don't care.)

Anyway, I can make this work with a hack on TSS side so, no worries, feel free to close.

As always, thanks for reviewing @mnajdova!

@mnajdova
Copy link
Member

Anyway, I can make this work with a hack on TSS side so, no worries, feel free to close.

Let's try to go with this first.

As always, thanks for reviewing @mnajdova!

Sure

@mnajdova mnajdova closed this Aug 25, 2022
@zannager zannager added package: system Specific to @mui/system core Infrastructure work going on behind the scenes labels Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants