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] Text wrap issue #38675

Open
oliviertassinari opened this issue Aug 27, 2023 · 20 comments · May be fixed by #42171 or #44907
Open

[Button] Text wrap issue #38675

oliviertassinari opened this issue Aug 27, 2023 · 20 comments · May be fixed by #42171 or #44907
Labels
component: button This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature good first issue Great for first contributions. Enable to learn the contribution process. package: joy-ui Specific to @mui/joy package: material-ui Specific to @mui/material

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 27, 2023

Steps to reproduce 🕹

Link to live example:

Steps:

  1. https://codesandbox.io/s/inspiring-browser-c7qmv7?file=/Demo.tsx

Current behavior 😯

Screenshot 2023-08-27 at 20 33 12

Expected behavior 🤔

Screenshot 2023-08-27 at 20 33 05

Context 🔦

It looks more correct on Material UI:

Screenshot 2023-08-27 at 20 38 13

https://codesandbox.io/s/peaceful-khorana-27gct9

but a lower line-height would I think feel better.

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.
@oliviertassinari oliviertassinari added component: button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material status: waiting for maintainer These issues haven't been looked at yet by a maintainer package: joy-ui Specific to @mui/joy labels Aug 27, 2023
@oliviertassinari oliviertassinari changed the title [Button] Text overflow issue [Button] Text wrap issue Aug 27, 2023
atharva3333 added a commit to atharva3333/material-ui that referenced this issue Aug 29, 2023
@siriwatknp siriwatknp added enhancement This is not a bug, nor a new feature good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 12, 2023
@sofiyanpathan
Copy link

Hi ,can I work on this if it is open?

@siriwatknp
Copy link
Member

Hi ,can I work on this if it is open?

Thanks for asking! The PR is already there, would you mind taking a look at other issues?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 18, 2023

I checked on Material UI v6, it looks ok: https://mui.com/material-ui/react-button/#material-you-version

Screenshot 2023-09-18 at 23 46 43

cc @DiegoAndai. I wouldn't mind -1px on padding-top and padding-bottom though:

Screenshot 2023-09-18 at 23 50 16

Actually, should we rename "Material You version" to something like "Material UI v6 (alpha)"?

Screenshot 2023-09-18 at 23 48 47

@Praveen03AJJARAPU
Copy link

Is this issue still open, so that I can work on this.

@akshayw1
Copy link

akshayw1 commented Oct 1, 2023

Hey,Is this issue still open, so that I can work on this.

@DiegoAndai
Copy link
Member

@Praveen03AJJARAPU @akshayw1 Thanks for asking, the Joy PR is already up. On the Material line height, we should define it before working on it.

but a lower line-height would I think feel better.

@oliviertassinari @zanivan, what should we use for the line height? Would we change it for buttons with text wrapping only?

Regarding v6 (#38675 (comment))

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 2, 2023

what should we use for the line height?

@DiegoAndai I'm not familiar with the implementation, to consider the impact on the design tokens. Maybe it's not worth it. In v5, If I recall the code correctly, this isn't a concern.

At the end of the day, I think the UX should win over other considerations, like following exactly the design token structure. I mean, we should aim to make the same tradeoffs designers & engineers would want to make.

Would we change it for buttons with text wrapping only?

I don't think developers or the code can know if a button wraps. In most cases, it's not supposed to (this issue is about minimizing the worst case scenario when a button needs to wrap to fit in the layout).

@hetpatel4381
Copy link

Hello is the issue still open can i pull and solve it ?

@ShaikhHeeba07
Copy link

Hello, I'd like to inquire if the issue is still available for me to pull and work on?

@zanivan
Copy link
Contributor

zanivan commented Oct 6, 2023

Hey, @ShaikhHeeba07 @hetpatel4381 thanks for asking! We already have two undergoing PRs for the Joy UI line height on buttons, the #38696 and #39316 :)

@siriwatknp siriwatknp reopened this Oct 9, 2023
@Riyanshi26
Copy link

Hello!
Is the issue still open? If yes, can I work on it?

@ArchitGajjar
Copy link

Hi @oliviertassinari - can you please assign this issue to me ?

@anuragsingh6886
Copy link

Hello, I'd like to know if this issue is still persist for me to pull and work on?

@sreeragdas
Copy link

hi , can i work on this

@geekngoyal
Copy link

Hi, let me know if I can start working on it. Looking to start my open source journey. Thanks

@siriwatknp siriwatknp removed their assignment Apr 10, 2024
@siriwatknp
Copy link
Member

@zanivan What should be the line-height for Material UI Button?

@zanivan
Copy link
Contributor

zanivan commented Apr 10, 2024

@zanivan What should be the line-height for Material UI Button?

On Material Design 2 the line-height is 1.5. Material Design 3 uses 1.25. Currently, if I'm not mistaken, we're using 1.75 on buttons. So, making it 1.5 would suit the MD2 guidelines and make spacing more consistent with padding, e.g.:

1.75 1.5
Screenshot 2024-04-10 at 10 15 16 Screenshot 2024-04-10 at 10 15 30

@siriwatknp
Copy link
Member

For anyone who is interested to submit a PR, here is the fix:

diff --git a/packages/mui-material/src/styles/createTypography.js b/packages/mui-material/src/styles/createTypography.js
index 277808d2a7..32b5e9f790 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.5, 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.

@successbyte
Copy link

successbyte commented May 6, 2024

For anyone who is interested to submit a PR, here is the fix:

diff --git a/packages/mui-material/src/styles/createTypography.js b/packages/mui-material/src/styles/createTypography.js
index 277808d2a7..32b5e9f790 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.5, 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.

Hi, I am interested to Contribute with MUI and it would be an start point if this issue is not assigned to anyone
Can I work on it?

successbyte added a commit to successbyte/material-ui that referenced this issue May 7, 2024
successbyte added a commit to successbyte/material-ui that referenced this issue May 7, 2024
successbyte added a commit to successbyte/material-ui that referenced this issue May 8, 2024
@successbyte successbyte linked a pull request May 9, 2024 that will close this issue
1 task
@successbyte
Copy link

Hello everyone
can you guide me how to pass the checks please!

kanavbajaj added a commit to kanavbajaj/material-ui that referenced this issue Dec 31, 2024
@samuelsycamore samuelsycamore linked a pull request Dec 31, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature good first issue Great for first contributions. Enable to learn the contribution process. package: joy-ui Specific to @mui/joy package: material-ui Specific to @mui/material
Projects
None yet