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

[Dialog] Fix action width issue #12081

Merged
merged 3 commits into from
Jul 9, 2018
Merged

Conversation

mim-Armand
Copy link
Contributor

@mim-Armand mim-Armand commented Jul 7, 2018

Closes #11837

Thank you for your feedback.

@mim-Armand
Copy link
Contributor Author

This is my first contribution to MU project and I couldn't run the tests ( it was complaining about missing packages and stuff! I read the documentation for Contribution, Code of conduct and testing instructions but with multiple package.json files I couldn't manage to make it happy again, so please let me know if I missed something ).
It's a very small fix so it could be all OK, but I would appreciate feedback if there is any issues.

Thanks! :)

@mim-Armand
Copy link
Contributor Author

@oliviertassinari Do we need to update the Argos snapshots for this or should it have gone through without any changes to the UI?

@@ -13,7 +13,7 @@ export const styles = theme => ({
...theme.typography.button,
lineHeight: '1.4em', // Improve readability for multiline button.
boxSizing: 'border-box',
minWidth: 88,
minWidth: theme.spacing.unit * 8,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've moved away from using theme.spacing.unit in components, but setting this to 64 would be a breaking change IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you are right, as theme.spacing.unit * 8 is being calculated to 64 already and that changes the rendering in a few component ( not sure if that's not expected )
I've changed it for now tho if we are not using theme.spacing.unit anymore.

@oliviertassinari oliviertassinari changed the title attemting to fix Issue #11837 [Dialog] Fix action width issue Jul 8, 2018
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: dialog This is the name of the generic UI component, not the React module! labels Jul 8, 2018
@oliviertassinari oliviertassinari self-assigned this Jul 8, 2018
@oliviertassinari
Copy link
Member

@mbrookes MCW is using 64px too. I think that this change is part of their new strategy, makes Material Design less opinionated.

capture d ecran 2018-07-09 a 10 02 00

@oliviertassinari oliviertassinari merged commit d205e2d into mui:master Jul 9, 2018
@oliviertassinari
Copy link
Member

@mim-Armand It's a great first pull request on Material-UI 👌🏻. Thank you for giving it a shot!

acroyear pushed a commit to acroyear/material-ui that referenced this pull request Jul 14, 2018
* attemting to fix Issue mui#11837 ( where the CircularProgress size were incorrect when rendered in a Dialog )

* changint the max-with to the calculated amount instead of using theme.spacing.unit * 8

* remove dialog action min-width
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug 🐛 Something doesn't work component: dialog This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants