Skip to content

[Button] Fix vertical text alignment by reducing padding #13931

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 25, 2018
Merged

[Button] Fix vertical text alignment by reducing padding #13931

merged 2 commits into from
Dec 25, 2018

Conversation

@adipascu
Copy link
Contributor

@adipascu adipascu commented Dec 17, 2018 •
edited by oliviertassinari
Loading

Fix #13926
Fix #13900

@adipascu
Copy link
Contributor Author

adipascu commented Dec 17, 2018 •
edited
Loading

Looks like the project has visual regression testing with Argos.
I am looking into it.

Edit: looks like there are no screenshots inside this repo, Argos is just comparing them with the previous revision. Leave a comment if you need anything before a merge.

@oliviertassinari oliviertassinari changed the title Fix text alignment by reducing button height [Button] Fix vertical text alignment by reducing padding Dec 17, 2018
@oliviertassinari oliviertassinari added type: bug It doesn't behave as expected. scope: button Changes related to the button. labels Dec 17, 2018
@adipascu
Copy link
Contributor Author

adipascu commented Dec 17, 2018 •
edited
Loading

I am closing this PR for now because regression-Button/MaterialV1Buttons.png looks worse after the changes. See the Argos build https://www.argos-ci.com/mui-org/material-ui/builds/38784 .

@adipascu adipascu closed this Dec 17, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 17, 2018

@adipascu I'm looking into it, could you reopen? At least, we need to fix the height issue.

@adipascu
Copy link
Contributor Author

adipascu commented Dec 17, 2018

Ok reopened, thanks for taking the time to look into it. I am logging out for the day.

@adipascu adipascu reopened this Dec 17, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 18, 2018

It should be great now:

capture d ecran 2018-12-18 a 20 56 20

@adipascu
Copy link
Contributor Author

adipascu commented Dec 18, 2018

The fix for #13926 (visually) looks good to me 👍
The rest (visual and code changes) are beyond my knowledge of this library.

@oliviertassinari If you want to add more code to this PR, consider replacing it with one authored by you.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 18, 2018

@adipascu Thank you. The list item changes solve a mismatch with the material specification.

@oliviertassinari oliviertassinari added this to the v3.8.0 milestone Dec 20, 2018
@oliviertassinari oliviertassinari merged commit 3a43aef into mui:master Dec 25, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 25, 2018

@adipascu It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@pelotom pelotom mentioned this pull request Dec 31, 2018
Closed
This was referenced Sep 15, 2021
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Dec 24, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

scope: button Changes related to the button. type: bug It doesn't behave as expected.

Projects

None yet

Milestone

v3.8.0

Development

Successfully merging this pull request may close these issues.

2 participants

@adipascu @oliviertassinari