mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.98k stars 32.29k forks source link

[material-ui] Inconsistent spacing units between components under default styling #40994

Open lpolito opened 9 months ago

lpolito commented 9 months ago

Steps to reproduce

Link to live example: https://codesandbox.io/p/sandbox/sad-forest-g88786?file=%2Fsrc%2Findex.tsx%3A35%2C3 You'll see there is red inner content surrounded by green outer content - the hope is that this illustrates where padding is used.

Current behavior

Spacing units is inconsistent between different components' default styles. This issue becomes very apparent when adding a custom spacing function to the theme.

When specifying space in the sx of any component the behavior is consistent and predictable

Unfortunately with the way default styles are defined on out of the box components, this custom spacing function completely breaks consistency on some base styles.

For example:

Button and CardContent's padding ends up matching what the original intent was and renders appropriately. AccordionSummary's padding ends up being completely wrong.

Expected behavior

My hope is that in the future there is consistency between component's base styles' use of units on spacing. It seems like unit-less spacing in these default styles would be preferred, so that in the end the spacing function is used and devs have more control over what that actually means. This seems like something that should be enforced on the project as a whole.

Context

Above is an example (and mirrors my project's production environment) where we convert all spacing units to rem, and base it on the fact the base website font size is 10px.

Your environment

No response

Search keywords: space spacing units theme

lpolito commented 9 months ago

I just found some additional inconsistent behavior.

I noticed the AccordionAction's default padding was padding: 8, but in my sandbox it is getting converted to padding: 8px even if my spacing function is completely different.

https://codesandbox.io/p/sandbox/sad-lichterman-zx33h7?file=%2Fsrc%2Findex.tsx%3A37%2C23

This is an extreme example where the spacing function is blown way up, but the only component that stays different / doesn't respect the spacing function is AccordionActions

Doesn't this imply the spacing function isn't getting passed to the default styles when there's just a number value provided to spacing properties? In contrast, AccordionSummary's use of theme.spacing() DOES use the provided spacing function, which tells me that it's at least accessible to the default style in some fashion.

This is very unpredictable.

siriwatknp commented 9 months ago

@lpolito Can you check the https://codesandbox.io/p/sandbox/sad-forest-g88786?file=%2Fsrc%2Findex.tsx%3A35%2C3? It says "Sandbox not found".

lpolito commented 9 months ago

@siriwatknp Apologies, it was private. It should work now.

siriwatknp commented 9 months ago

We can definitely improve this in v7 to make the experience consistent across all components.