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
91.86k stars 31.57k forks source link

[material-ui] Cannot globally override `margin`/`margin-*` CSS properties of elements represented by `Typography` #42071

Open raja-s opened 2 weeks ago

raja-s commented 2 weeks ago

Steps to reproduce

Link to live example: https://stackblitz.com/edit/github-csxp7s?file=src%2Findex.tsx

Steps: (following the instructions in the documentation)

  1. Create a theme with a components.MuiCssBaseline.styleOverrides field and override the margin of a text element, e.g. h3
  2. Of course, add <ThemeProvider>...</ThemeProvider> with the theme and <CssBaseline />
  3. Add a text element of the type whose margin was overriden in step 1. and add some other content around it for reference

Current behavior

The margin specified in the CSS override has no effect: actual-behavior

Expected behavior

The margin specified in the CSS override is applied: expected-behavior

Context

I have a feeling this is what was being complained about in https://github.com/mui/material-ui/issues/30360, although the poster there seemed to be getting into implementation details and I see the discussion led nowhere.

I'm presenting the issue to you from a user's perspective: according to this part of the documentation (second example with CssBaseline) I should be able to globally override any CSS property of any element in my custom theme, inside components.MuiCssBaseline.styleOverrides, but the example conveniently uses color which does work, except that margin does not, which took me a little investigating to figure out. Overriding any of the margin-* properties doesn't work either as the margin property in .css-...-MuiTypography-root still takes precedence.

I haven't tested it but I can tell from the content of these generated .css-...-MuiTypography-root classes that font-family, font-weight, font-size, line-height and letter-spacing all wouldn't work here either. BUT, mui gives us a way to override all of those in the theme, just somewhere else, under typography.h1, typography.h2, etc. So only margin is really a problem here.

The only workarounds I've found so far are:

But I think they're both pretty dirty. I'd like mui to provide me with a proper way to set global margins for the different text elements.

Your environment

npx @mui/envinfo ``` System: OS: Linux 6.8 Arch Linux Binaries: Node: 18.20.2 - ~/.nvm/versions/node/v18.20.2/bin/node npm: 10.5.0 - ~/.nvm/versions/node/v18.20.2/bin/npm pnpm: Not Found Browsers: Chrome: Not Found npmPackages: @emotion/react: ^11.11.4 => 11.11.4 @emotion/styled: ^11.11.5 => 11.11.5 @mui/base: 5.0.0-beta.40 @mui/core-downloads-tracker: 5.15.15 @mui/icons-material: ^5.15.15 => 5.15.15 @mui/material: ^5.15.15 => 5.15.15 @mui/private-theming: 5.15.14 @mui/styled-engine: 5.15.14 @mui/system: 5.15.15 @mui/types: 7.2.14 @mui/utils: 5.15.14 @types/react: ^18.2.66 => 18.2.79 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.4.5 => 5.4.5 ```

On my side I reproduced this on both Chromium (124.0.6367.60) and Firefox (125.0.1).

Search keywords: typography margin css override

raja-s commented 2 weeks ago

Btw, playing around with this, I've found that overriding h3.MuiTypography-h3 works, not the tag alone h3, not the class alone .MuiTypography-h3, but both of them together will create a rule that takes precedence over the .css-...-MuiTypography-root class generated specifically for the instance of the element. Is this the "idiomatic" way to do it?

I've seen these CSS classes before in the documentation, they seem to be part of the API and so can be used safely for styling without risk of changing in the future. Although it is still not entirely clear to me how reliable this method of global overrides is, for example I just ran into another case where, while overriding button.MuiButtonBase-root works for buttons in some cases, it will still be overriden by an "instance rule" .css-...-MuiStack-root when the button is in a <Stack>...</Stack>: second-behavior

(I'm learning mui as I go, plus I don't have that much frontend experience to begin with, so I may be missing important points on how to do styling properly...)

raja-s commented 2 weeks ago

Ok one last comment (sorry for the long thread and it's only been me so far :p).

I quickly realized that this "workaround" with h3.MuiTypography-h3 does not really solve the issue because if the h3.MuiTypography-h3 rule takes precedence over the .css-...-MuiTypography-root rule, then I can no longer override the margin for a specific element for the exact same reason. This made me stop for a minute and think about the whole problem and what it is I'm trying to achieve and what my expectations are from a framework like mui.

On a high level, I would like to be able to specify a default (maybe "default" is a better word here than "override") margin for elements of a certain kind. This is purely to avoid repetition, cause I find myself adding text/buttons a million times and every time specifying a margin, and 95% of the time the margin is the same for the same kind of element (h1, h2, etc.) so to me it's only natural to be able to specify this margin as a default value for that kind. It's also useful in case I want to make consistent small adjustments to the margins, I can do it in one place rather than scouring the codebase looking for all those margins. For the remaining 5% of the time though, I also want to be able to easily override the margin for a specific element where the default value is not suitable.

Now what this means concretely with how HTML/CSS/MUI works, I'm not entirely sure, but from what I can see it seems to me like I have a problem specifically with the .css-...-MuiTypography-root rule (not just for typography elements but other kinds of elements as well) specifying a margin, cause if I understand correctly one of these classes is generated per element, so practically it's as if each element had an id and these were the id-specific (#id {...}) rules, so they are meant to be the most specific, and I understand that they come pre-filled with some properties in order to implement the default styles of mui (after all, it's a collection of ready-to-use styled components), but it ends up preventing me from achieving the (in my view, fairly simple) goal that I described above: global defaults, specific overrides. Again, not sure what this means on an implementation level with how CSS works, but I guess I somehow expect a precedence system as folllows:

                           user-defined   /     user specific overrides       ^ highest precedence
                            properties    \       user global defaults        |
                                                                              |
I have no idea how any of this actually   /  the most specific mui overrides  |
works. I imagine mui has a whole          | some less specific mui overrides  |
hierarchy of rules to implement the       |                  .                |
default styles (e.g. generic container    |                  .                |
> ... > button). The point is, all of     |                  .                |
these would be below BOTH user-defined    |  some more specific mui defaults  |
defaults AND overrides.                   \    the most global mui defaults   v lowest precedence

That's the way I see it, but again, I may be approaching the problem the wrong way or missing some details on how to do styling properly in CSS/MUI, or maybe such a precedence system is not possible in practice cause it creates problems elsewhere that I haven't considered... Let me know your thoughts :)

mnajdova commented 3 days ago

Can you please update the CodeSandbox to illustrate what the problem actually is about? In general in CSS the presedence is: tag selector < classname < id, so what you shared makes sense, the MuiTypography styles win over the styles specified for the tag.

I may have misunderstood your question, but I would recommend going over this.