mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
92.52k stars 31.89k forks source link

[ExpansionPanel] Cannot override margins with 'classes' prop #11223

Closed adam-nielsen closed 6 years ago

adam-nielsen commented 6 years ago

I am trying to make the expanded summary area of the ExpansionPanel much thinner, but I have found that the necessary styles cannot be overridden as normal. It seems that a more specific CSS class is used, which always overrides anything you specify.

Expected Behavior

The margin and min-height CSS values should apply to the element.

Current Behavior

Regardless of what you specify, margin is always 20px 0 and min-height is always 64px.

Steps to Reproduce (for bugs)

Codesandbox example

const styles = theme => ({
  expanded: {
    backgroundColor: 'red',
    minHeight: 0,
    marginTop: 0,
    marginBottom: 0,
  },
});
<ExpansionPanelSummary classes={{expanded: classes.expanded}}>

backgroundColor gets applied to the expanded summary area, but the other listed CSS attributes get overridden.

expansionpanel-root

expansionpanel-content

Context

I am trying to make the expanded summary area the same size as the compressed area, and both of them smaller than the default (by removing the padding).

Your Environment

Tech Version
Material-UI 1.0.0-beta.44
browser Firefox and Chrome
oliviertassinari commented 6 years ago

The same problem has been discussed in https://github.com/mui-org/material-ui/pull/10961#issuecomment-385054841. I guess we lack some learning material about it. https://codesandbox.io/s/nwzmq0lzyj

adam-nielsen commented 6 years ago

Ah I see, thanks for the link.

I'm still struggling a bit to get that solution to work. If I do this:

const styles = theme => ({
  expandedSummary: {
    '&$expanded': {
      minHeight: 0,
      marginTop: 0,
      marginBottom: 0,
      backgroundColor: 'red',
    },
  },
});

<ExpansionPanelSummary classes={{expanded: classes.expandedSummary}}>

Then it doesn't work, but if I rename expandedSummary to expanded then it does work! Why would this make a difference? It seems that if the name of my internal object is important then something must be broken and it is only working by luck.

Here's a demo of the issue:

Example

This example does nothing to the style unless you rename expanded2 to expanded in both places, then the panel summary goes red (and shrinks) upon expansion.

I suspect the object name is being used to construct the CSS class name, but if that's true then I should be able to omit the classes={} line from the JSX, however this is still necessary. It seems wrong to have to specify a value that cannot be changed anyway.

Please correct me if I'm misunderstanding something here. Thanks again for your help!

oliviertassinari commented 6 years ago

Then it doesn't work, but if I rename expandedSummary to expanded then it does work! Why would this make a difference?

Your example is wrong. $expanded reference a local rule within the style sheet. This rule doesn't exist.

@kof maybe JSS should warn about missing referenced local rule?

Renaming the rule makes the reference works. You will ended up with a .foo.foo CSS in the DOM. This duplicated selector increase the specificity and override the style of Material-UI as injected after.

oliviertassinari commented 6 years ago

I have added a documentation section about this point as it was raised by 3 different people in a short period of time. I hope it will make things simpler to understand: #11227.

kof commented 6 years ago

@kof maybe JSS should warn about missing referenced local rule?

It does warn! https://github.com/cssinjs/jss-nested/blob/master/src/index.js#L19

oliviertassinari commented 6 years ago

@kof 🙏 Awesome, then people should just check their console :)

oliviertassinari commented 6 years ago

capture d ecran 2018-05-04 a 13 08 47

adam-nielsen commented 6 years ago

Ah right, sorry I didn't notice Codesandbox had a console! @kof Might I humbly suggest changing %s to "%s" in the warning? If I had seen the error Could not find the referenced rule expanded in xxx I would be looking in the wrong place for something being expanded, I wouldn't have realised that expanded was actually the name of the problematic rule!

Thanks also @oliviertassinari for the explanation of what $ is for. I have looked through the JSS docs but still haven't found where its purpose is explained! Are there any docs that go into more detail? Maybe I'm just looking for the wrong terms - searching for dollar and $ don't bring anything up.

Thanks again for your help!

kof commented 6 years ago

@adam-nielsen you might not just suggest but send a PR :)

felipezacani commented 5 years ago

I'm sorry but I think there's still an issue with the expansion panel. I read through the (new) documentation and followed your instructions carefully.

What I'm trying to do: customize the height of a the expansionpanelsummary, in both states (normal/expanded). So, in my understanding, I have to override two classes to achieve this: root (normal state) and expanded (expanded state). The first thing I noticed after I did it is that the expansion icon behavior gets weird. The second, it makes the summary text "jump" when the panel expands, and nothing else.

Anyway, I don't understand why would someone want to increase the summary height when the panel is expanded. The text moves down a little, its not an expected behavior.

By watching closely, you can see that the customization is effective by the duration of the panel expansion animation. After that, default values are applied.
oliviertassinari commented 5 years ago

@felipezacani Are you saying that https://material-ui.com/demos/expansion-panels/#customized-expansion-panel doesn't help?

felipezacani commented 5 years ago

@oliviertassinari Thanks. It does help indeed. I was focused on the overrides doc and completely skipped that. Really sorry for that.

However, does this mean there is no way to achieve the same thing with styles and classes, as documented in the overrides section? I'm ok with your solution, except that it looks completely alien for anyone looking at the rest of the project. I admit I'm a (little?) picky with my code, but I'm considering modifying all the rest of the code to use the same technique.

oliviertassinari commented 5 years ago

@felipezacani The underlying logic is the same. You should be able to port the styles to a theme.overrides approach without any effort.