Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Button] small button text not vertically centered with Roboto #29965

Open
apokusin opened this issue Nov 30, 2021 · 15 comments
Open

[Button] small button text not vertically centered with Roboto #29965

apokusin opened this issue Nov 30, 2021 · 15 comments
Assignees
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process

Comments

@apokusin
Copy link

apokusin commented Nov 30, 2021

Current behavior 😯

The text inside of a button with size="small" is not centered vertically. This is especially obvious when used with an icon or a contained/outlined button.

Expected behavior 🤔

The text inside of a button with size="small" is centered vertically.

Steps to reproduce 🕹

Steps:

  1. Use <Button size="small">text</Button>
  2. Observe text

Context 🔦

Codesandbox link

Screenshots:
Medium (alignment ok)
medium

Small (text too high)
small

Your environment 🌎

`npx @mui/envinfo`
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.
@apokusin apokusin added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 30, 2021
@mnajdova mnajdova added component: button This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 30, 2021
@mnajdova
Copy link
Member

I can notice it. It could be a regression after the span wrapping the text was removed. @danilo-leal could you double-check this please?

@apokusin
Copy link
Author

apokusin commented Nov 30, 2021

The CodeSandbox I've linked to above uses MUI 5.2.2, but I'm also seeing the issue as far back as 5.0.0-rc.0.

@danilo-leal
Copy link
Contributor

I suppose it could be related to the text line-height. The tricky thing is that it seems the overall Button height is composed of the text line-height. In the below example, the texts are without line-hight which makes the Buttons smaller. They seem to be centered though.

Screen Shot 2021-11-30 at 16 38 21

If we were about to remove the typography variant button line-height, we should probably add a minHeight to each component size variant so they don't shrink. Does that make sense?

@siriwatknp
Copy link
Member

siriwatknp commented Dec 15, 2021

If we were about to remove the typography variant button line-height, we should probably add a minHeight to each component size variant so they don't shrink. Does that make sense?

At this point, I suggest only adjusting to line-height: 1.5 for size small because changing to use minHeight in v5 might be a breaking change. In v6, we can revisit the size implementation again.

@siriwatknp siriwatknp added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 16, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 17, 2021

I had noticed the same problem, on macOS 12, Chrome v96! I'm glad we have an issue open about this problem. @apokusin thanks for the care ❤️ .

The large text and outlined variant feels a bit off too:
Screenshot 2021-12-17 at 11 17 35
Screenshot 2021-12-17 at 11 17 41

@shagungarg0
Copy link

can i work on this issue??

@abhinav-22-tech
Copy link
Contributor

can i work on this issue??

#30240 This is already on hold.

@shagungarg0
Copy link

can i work on this issue??

#30240 This is already on hold.

I am a beginner and target gsoc this year..can you guide me that how i should start??

@danilo-leal
Copy link
Contributor

Hey @shagungarg0, thanks for the willingness to start working on this issue. However, @abhinav-22-tech already took the lead and opened the PR for it but given it is a breaking change, we put it on hold until the next major (can't give you a prediction when that will happen exactly though).

To find more good opportunities to contribute and learn, you can filter the issues with good to take and good first issues labels and look around if any of them get you interested!

@danilo-leal danilo-leal removed their assignment Mar 11, 2022
@sajid19
Copy link

sajid19 commented Aug 10, 2022

I am new to the contribution. I want to work in this issue. May I?

@gauravsoti1
Copy link

@siriwatknp Should this still have a good first issue label? Won't it be better to have logos like v6 and on hold?

@danilo-leal
Copy link
Contributor

Won't it be better to have logos like v6 and on hold?

Yup, I guess so! Thanks for the heads up @gauravsoti1!

@danilo-leal danilo-leal added v6.x on hold There is a blocker, we need to wait and removed good first issue Great for first contributions. Enable to learn the contribution process. labels Aug 12, 2022
@oliviertassinari oliviertassinari removed on hold There is a blocker, we need to wait v6.x labels Sep 18, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 18, 2023

Removing the "on hold" label @DiegoAndai started the work on Material UI v6.


It feels 1px off too in https://mui.com/material-ui/react-button/#material-you-version

Screenshot 2023-09-18 at 23 19 57

With 1px less on padding-bottom:

Screenshot 2023-09-18 at 23 22 16

Now, the problem seems to be Roboto font related. If change the font to "IBM Plex Sans", it looks great by itself, same if I use the system font on macOS.

I suspect we need that https://web.dev/css-size-adjust/#descent-override

Screenshot 2023-09-18 at 23 37 55

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 18, 2023
@DiegoAndai
Copy link
Member

@oliviertassinari should we fix this for v6 only? or for v5 as well?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 20, 2023

descent-override: 15%; seems to be close to do the trick

@font-face {
  descent-override: 15%;
  font-family: "Roboto";

(after)

Screenshot 2023-09-20 at 14 23 53

https://codesandbox.io/s/iconlabelbuttons-material-demo-forked-8vtvv3?file=/public/global.css:34-56

vs. (before)

Screenshot 2023-09-20 at 14 24 16

https://codesandbox.io/s/iconlabelbuttons-material-demo-forked-m96yo?file=/demo.tsx

Now, the challenge is, I don't see how we can customize descent-override with Google Font CDN or Next.js https://nextjs.org/docs/app/building-your-application/optimizing/fonts. I have open google/fonts#6739.


should we fix this for v6 only? or for v5 as well?

@DiegoAndai At this point, I think we could focus most of our energy on v6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
Status: Stalled
Development

Successfully merging a pull request may close this issue.

10 participants