[ExpansionPanel] Use theme.spacing in summary #20344
Conversation
|
@eps1lon The motivation is about enabling custom values of Examples of obvious cases: Grid.spacing, Box.margin. |
What is the concern? |
That is looks broken. |
Why does this concern not apply to color, or font, or component props? |
Details of bundle changes.Comparing: 5aad5f2...26efdb1 Details of page changes
|
|
I believe there are 3 types of spacing, 1. the spacing between elements, which should and can be configured, 2. the spacing inside the element which should be immutable if it risks breaking the display. I'm not aware of the same constraint with colors or fonts. |
Why? This directly contradicts my understanding of spacing as well as the material guidelines. |
As far as I know, it doesn't work (as a systematized strategy). We can't change the spacing value and have it still look great everywhere. To solve this problem, the size enums were introduced, for instance: small, medium, large. Values with spacing we fully control. There is another dimension to the problem, developers might work with different spacing grid than Material Design. Material Design requires a 4px or 8px linear (depending on the context) grid. But people might very well use different values or non-linear e.g. |
|
Regarding this very component, I think that it could make sense to apply the change. |
Let's switch to I or you here. Otherwise this gets confusing. I don't expect components to look great if the spacing value changes. Just like I don't expect components to look great if color, or font, or props change. You seem to expect changes in color to not affect that a component looks great. How is that?
This is why we introduced overriding |
|
This topic is important if we're considering moving to a stricter handling of the theme (you suggest theme-ui in the last meeting). There's a very clear specification that changing the spacing values of the theme affects the spacing of components. I still don't understand why we have spacing in the theme if we shouldn't use it. |
|
@eps1lon I believe the convention in #12022 is:
I agree it's an important concern, it deserves more exploration. There are a couple of questions I wonder about that I think it would be awesome to answer:
Maybe we need an RFC and ask feedback from the community on Twitter? |
Personally I use it in the literal sense: "How much space does my UI have?". Larger value > more empty space, Smaller value > less empty space. In other words: This is a slider for the density. |
packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
Outdated
Show resolved
Hide resolved
oliviertassinari
added
PR: ready to ship
component: accordion
oliviertassinari
changed the title
This is a partial revert of #12022 to get a discussion starting why this was done since the original discussion was kept internal.
This is no longer consistent with our density guide.
4. It's not important to land this in a minor. It's probably safer anyway to do this in v5
/cc @oliviertassinari, @mbrookes