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

[material-ui][Grid] Update Grid props to match PigmentGrid #42742

Open
wants to merge 16 commits into
base: next
Choose a base branch
from

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Jun 24, 2024

Summary

This PR updates the Grid v2 API to match the upcoming Pigment Grid, so they are interchangeable.

Changes

Argos failures

The Argos failures are expected as the docs demos labels changes, but please review them as well to check if there's anything you wouldn't expect.

Not covered

There's room for improvement in the Grid's docs structure, and we should also discuss if we should stabilize it, but the scope of this PR is already big so I don't want to cover these here. I created issues for this:

@DiegoAndai DiegoAndai self-assigned this Jun 24, 2024
@DiegoAndai DiegoAndai force-pushed the grid-v2-props branch 5 times, most recently from 78911f1 to a416448 Compare June 25, 2024 20:01
@DiegoAndai DiegoAndai force-pushed the grid-v2-props branch 2 times, most recently from 35b31ba to 1eae06c Compare June 26, 2024 13:47
@DiegoAndai DiegoAndai marked this pull request as ready for review June 26, 2024 16:19
@DiegoAndai
Copy link
Member Author

DiegoAndai commented Jun 26, 2024

@danilo-leal may I ask you to review the marketing pages updates? To check everything is still working as expected 😊

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 This is awesome, well done @DiegoAndai.

One comment from me is the Argos. I think the demos should not change so that it verifies the implementation change. To me, it's fine to show xs=8 instead of size=8 etc.

@DiegoAndai DiegoAndai force-pushed the grid-v2-props branch 2 times, most recently from 23dfe2b to 5e5e358 Compare June 28, 2024 14:55
@DiegoAndai DiegoAndai force-pushed the grid-v2-props branch 3 times, most recently from 09603dd to 7b2bb15 Compare June 28, 2024 16:35
@DiegoAndai
Copy link
Member Author

I think the demos should not change so that it verifies the implementation change

I pushed a temporary commit (b3b22f7) so that it runs the Argos tests and verifies the implementation, and the test succeeded: https://app.argos-ci.com/mui/material-ui/builds/29397/96799521

To me, it's fine to show xs=8 instead of size=8 etc.

I think the updated labels match better with the new API, though, so after the successful test, I reverted the changes back (186c18e)

This way, we have the improved demos, and we verified with Argos, best of both worlds 😉


Also, I had to do some minor refactoring (f5b6746) after #42693 was merged. In case you want to review it again.

@siriwatknp

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

3 participants