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.

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

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

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 bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! 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

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

@adipascu
Copy link
Contributor Author

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

It should be great now:

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

@adipascu
Copy link
Contributor Author

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

@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

@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
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@adipascu @oliviertassinari