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

[Grid] Add alignItems & alignContent properties #8647

Merged
merged 7 commits into from Oct 12, 2017
Merged

[Grid] Add alignItems & alignContent properties #8647

merged 7 commits into from Oct 12, 2017

Conversation

sakulstra
Copy link
Contributor

@sakulstra sakulstra commented Oct 10, 2017

@oliviertassinari not sure if it makes sense to add alignContent to the interactive demo - at least not in the current format, as alignContent won't make any difference.

Breaking change

-        <Grid container xs={6} align="flex-end">
+        <Grid container xs={6} alignItems="flex-end">
           <Grid item>

Closes #8599

@oliviertassinari oliviertassinari changed the title Feat/grid align [Grid] Add alignItems & alignContent properties Oct 11, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 11, 2017

alignContent won't make any difference

@sakulstra It needs multiple lines to make a difference. I don't think that we should add it for a simple reason, more options are more overhead for our users and decrease the demo quality. Sure we could have changed it so the alignContent makes a difference, but I think that it misses the points.

Regarding visual regression tests, I think that it's optional, we can be lazy on this feature and wait regression/bugs to add some.

Looks good otherwise, good job 👍

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed v1 labels Oct 11, 2017
@sakulstra
Copy link
Contributor Author

@oliviertassinari i never worked with a tool like argos, so i want to fix it as a learning experience. The change on InteractiveGrid is intended(alignItems instead of align), but why is PositionedTooltips broken on argos? locally it seems to be the same like before the change.

@sakulstra
Copy link
Contributor Author

sakulstra commented Oct 11, 2017

🙊 i didn't realize there was an actual test file for that :/ only looked at the docs 👍
Thanks for helping me out!

@oliviertassinari oliviertassinari added PR: accepted component: Grid The React component. and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Oct 11, 2017
@oliviertassinari
Copy link
Member

I have forgotten my laptop at the office. I can't merge anything before tomorrow.

@oliviertassinari oliviertassinari merged commit fc7ac72 into mui:v1-beta Oct 12, 2017
@oliviertassinari
Copy link
Member

@sakulstra Thanks :)

the-noob pushed a commit to the-noob/material-ui that referenced this pull request Oct 17, 2017
* breaking(grid-align): rename align to alignItems

* feat(grid): added alignContent prop

* fix docs

* fix(docs): new naming

* fix tooltip docs :/

* remove alignContent from interactive demo and add it to api docs

* fix regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants