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.96k stars 32.27k forks source link

[link] Link component adds additional class with custom styles without any reason #30139

Open igaponov opened 2 years ago

igaponov commented 2 years ago

Current behavior 😯

Link component adds a class named css-XXXXXXX-MuiTypography-root-MuiLink-root to "a" tag with custom styles, this breaks all my markup because it has "margin: 0" rule and it always comes last and rewrites all other styles. The documentation doesn't mention this behavior.

<a class="MuiTypography-root MuiTypography-inherit MuiLink-root MuiLink-underlineAlways css-1iwtne7-MuiTypography-root-MuiLink-root" href="/">Link</a>

Expected behavior 🤔

A tag without additional class

<a class="MuiTypography-root MuiTypography-inherit MuiLink-root MuiLink-underlineAlways" href="/">Link</a>

Steps to reproduce 🕹

Steps:

  1. Create new project
  2. Add Link component

https://codesandbox.io/s/react-mui-forked-0sewm

Context 🔦

No response

Your environment 🌎

`npx @mui/envinfo` ``` System: OS: Linux 5.13 Ubuntu 21.10 21.10 (Impish Indri) Binaries: Node: 16.13.1 - /usr/bin/node Yarn: 1.22.15 - /usr/bin/yarn npm: 8.1.2 - /usr/bin/npm Browsers: Chrome: 96.0.4664.93 Firefox: 94.0 npmPackages: @emotion/react: ^11.7.0 => 11.7.0 @emotion/styled: ^11.6.0 => 11.6.0 @mui/base: 5.0.0-alpha.59 @mui/material: ^5.2.3 => 5.2.3 @mui/private-theming: 5.2.3 @mui/styled-engine: 5.2.0 @mui/system: 5.2.3 @mui/types: 7.1.0 @mui/utils: 5.2.3 @types/react: ^17.0.0 => 17.0.37 react: ^17.0.2 => 17.0.2 react-dom: ^17.0.2 => 17.0.2 typescript: ^4.1.2 => 4.5.3 ```
mnajdova commented 2 years ago

That class is generated by emotion and contains all styles that MUI defines for this component. One of that style is the margin. See:

https://github.com/mui/material-ui/blob/ea9fd413e1cd0678e553bf323f4d72b7bd1aed08/packages/mui-material/src/Typography/Typography.js#L45

Which styles does it rewrite? What are you trying to solve?

igaponov commented 2 years ago

@mnajdova it rewrites my custom margins. Is it possible to disable it? Or move those styles before my custom ones?

igaponov commented 2 years ago

@mnajdova I've updated the example in https://codesandbox.io/s/react-mui-forked-0sewm so you can see, that the margin for the Link component in App component is rewritten by that custom class css-1iwtne7-MuiTypography-root-MuiLink-root.

mnajdova commented 2 years ago

Ideally none of the components would have a margin, but in this case we are setting to do some resets and have a consistent look in all browsers. Global styles are always injected first, and you cannot change this. I would propose using a selector for the list in your global styles, for example:

  margin: 10px;
  & .MuiLink-root {
    margin: 10px;
  }
mnajdova commented 2 years ago

The codesandbox is not working, the App.js is empty.

igaponov commented 2 years ago

Fixed codesandbox. I have custom margins, adding it to a parent selector for every link will require a lot of work, that's definitely not a nice BC.

mnajdova commented 2 years ago

The issue here is that you are using makeStyles with v5. If you wish to do this you need to have StyledEngineProvider with injectFirst in the root of your app. See the index.js file in https://codesandbox.io/s/react-mui-forked-j6uqf?file=/App.js All works as expected now. I hope this helps.

montogeek commented 2 years ago

It is possible to remove the other classes that doesn't have any styles defined from the HTML?

mnajdova commented 2 years ago

It is possible to remove the other classes that doesn't have any styles defined from the HTML?

Currently not, but we discussed this at some point with @michaldudak. I would be curious to know what is the reason you would want that @montogeek

montogeek commented 2 years ago

@mnajdova To keep the HTML clean

mnajdova commented 2 years ago

Alright, fair enough, cc @michaldudak @siriwatknp I am adding this to the v6 milestone so that we can make a decision on this going forward.

rcb4t2 commented 2 years ago

@mnajdova The docs for Link component says that it also accepts Typography props. The props gutterBottom and paragraph do not work because the margin: 0 is baked in as described by OP. Likewise, using sx: {{ mb: SOME_NUMBER }} can't apply a margin bottom either

Applying a bottom margin to a text/link component is a pretty common and reasonable need

Please consider addressing this as a v5 patch. Thanks!

rcb4t2 commented 2 years ago

For anyone else looking at this, a decent workaround is like so:

<Link href='http://domain.com'>
    <Typography variant='body1' gutterBottom>The Link</Typography>
</Link>

That way you can still get the built-in Link styles and apply Typography props, including bottom margin

Not a great DX though

mnajdova commented 2 years ago

Setting margin: 0 is indeed strange, it creates more problems than what it solves. We should look into whether they are ways to get rid of it. Would anyone want to take a stab at investigating this and propose a PR so that we can test the changes in more browsers?

rcb4t2 commented 2 years ago

textAlign props don't really work great on links either, because they're inline maybe?

I think a larger overhaul of the Link/Typography connection is needed. It's convenient to have Link accept Typography props but there are a lot of undocumented edge cases that don't work as expected

oliviertassinari commented 8 months ago

I see no root problems on this issue related to the <Link>.

How about we close it?