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.86k stars 32.26k forks source link

[material-ui][Button] Text not aligned vertically with icon #19584

Open NZainchkovskiy opened 4 years ago

NZainchkovskiy commented 4 years ago

When button has an icon, text is not aligned vertically. You can see it at example page. image I made an screenshot and check at graphics designer - button "delete" has 12px from top to text and 14px from text to bottom. While icon has exactly 10px from top and from bottom.

oliviertassinari commented 4 years ago

We rely on flexbox vertical alignment. I doubt we can do much about it.

hmillison commented 2 years ago

Can someone look into this? It feels wrong that the default icons provided with Material UI used with the components aren't aligned properly.

irfancankalelikargo commented 2 years ago

Still an issue, button with icons looks vertically aligned on Firefox but not in Chrome

forgo commented 2 years ago

We rely on flexbox vertical alignment. I doubt we can do much about it.

I agree MUI should not be responsible for centering the font itself, as fonts vary wildly in how they handle ascender/descender dimensions; however, there still seems to be something off that I can't perfectly quantify.

A wrapper for the text doesn't actually help much because the height of a button is unconstrained and fits to what you put inside. So, if I change the font-size to 30px in a "small" variant button, all the "small" really relates to is the padding and icon size. This does not actually constrain the height of the button itself. Having a constrained height of the button might make this an easier problem to solve, but I realize that could potentially break a lot of assumptions in a lot of code for a lot of people.

A "small" button with 30px font size:

Screen Shot 2021-12-23 at 4 08 53 PM

I'm more or less just pointing out the issue at hand, and why it's so frustrating.

Screen Shot 2021-12-23 at 3 48 01 PM

When focusing the browser on the text alone, at least some of the misalignment issue is seen to come from the font itself -- as the gap below the text is part of the highlighted area:

Screen Shot 2021-12-23 at 3 49 30 PM

Yet... that's not the whole picture. The icon is not completely center because the SVG path itself may not be centered perfectly in the 24x24 viewbox, even if the SVG conforms to the recommended 24x24 pattern.

Regardless, something seems off in how the icon and text are aligned within the button. They seem more off-center than the offsets of either the SVG or the text would lead you to believe, which makes me think there could be some CSS improvements on the MUI side, regardless.

Screen Shot 2021-12-23 at 3 51 11 PM

I'm honestly not sure how to address this issue without completely re-writing the structure and styles of all the button parts and pieces -- which is not ideal.

Ideally, because there are so many kind of fonts out there, the best thing MUI could do is provide some kind of "verticalOffsetContent" and "verticalOffsetIcon" prop that does not actually affect the size of the button itself, but simply shifts the content of the button. I have not been very successful doing this manually, so I'm not confident it would be an easy fix on the MUI side.

bradstdev commented 2 years ago

I found this helped in the theme:

      MuiButton: {
        styleOverrides: {
          root: {
            lineHeight: 0
          }
        }
      },
YakuBrangJa commented 2 years ago

Here is my solution ``

MuiButton: { styleOverrides: { root: { gap: '0.5rem', } } },

owenashurst commented 2 years ago

It seems like the lineHeight like @brstewart mentioned. Removing this seems to align them perfectly.

aaron-mota commented 2 years ago

@brstewart answer also overwrites other size buttons (e.g. "medium"), and breaks those buttons aesthetically. Is there a way to apply this to only size="small" buttons?

DiegoAndai commented 2 months ago

We have a new report about this bug, now showing in Windows but not macOS: https://github.com/mui/material-ui/issues/43545.

I can confirm this is present in the latest versions of v5 and v6, so I'm reopening this one and adding the ready-to-take label.

Finally, I'm adding it to the v7 milestone so we remember to cover it when implementing Material Design 3. This doesn't mean we'll have to wait until v7; hopefully, we'll get a fix sooner. It's only a reminder, so we fix it once and for all by, at most, v7.

The live repro link is still the demo: https://mui.com/material-ui/react-button/#buttons-with-icons-and-label

oliviertassinari commented 2 months ago

It looks very close to #29965 for macOS. Do we consider those duplicates or we use #19584 for Windows?

The descent-override CSS property works on Windows too: https://github.com/mui/material-ui/issues/29965#issuecomment-1727652056 but it's hard to deploy. A simpler solution seems to be:

diff --git a/packages/mui-material/src/styles/createTypography.js b/packages/mui-material/src/styles/createTypography.js
index 277808d2a7..11210f652f 100644
--- a/packages/mui-material/src/styles/createTypography.js
+++ b/packages/mui-material/src/styles/createTypography.js
@@ -69,7 +69,7 @@ export default function createTypography(palette, typography) {
     subtitle2: buildVariant(fontWeightMedium, 14, 1.57, 0.1),
     body1: buildVariant(fontWeightRegular, 16, 1.5, 0.15),
     body2: buildVariant(fontWeightRegular, 14, 1.43, 0.15),
-    button: buildVariant(fontWeightMedium, 14, 1.75, 0.4, caseAllCaps),
+    button: buildVariant(fontWeightMedium, 14, 1.7, 0.4, caseAllCaps),
     caption: buildVariant(fontWeightRegular, 12, 1.66, 0.4),
     overline: buildVariant(fontWeightRegular, 12, 2.66, 1, caseAllCaps),
     // TODO v6: Remove handling of 'inherit' variant from the theme as it is already handled in Material UI's Typography component. Also, remember to remove the associated types.

which works with the couple of fonts that I could test on Windows and macOS. I have no idea why this works 🙃