-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Container] Add disableGutters prop #15872
[Container] Add disableGutters prop #15872
Conversation
Details of bundle changes.Comparing: a973b2f...75f0d16
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great but I have one concern with the breaking change potential for somebody changing the padding values with the theme.overrides. Should we care?
Yup, it will be a breaking change for such use cases. Assuming that we can't assess the impact of this, we can probably hold this up until the next major release. It will be great to have other's opinions about this. |
@divyanshutomar The next major release won't happen before a year. |
@divyanshutomar Ok, we can wait for v5. Do you think that you could split the pull request in 2? The CSS update is great to take on its own :). |
@oliviertassinari Sure, I will send a PR that uses gutters class to add padding to the existing Container component. And, update this one with the disableGutters props changes. |
c1649b4
to
29ee2bd
Compare
Rebased. |
29ee2bd
to
75f0d16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the approach to be nonbreaking, I start to even think that it's a better approach. I think that we can move forward with it.
@divyanshutomar I'm sorry it took so long. Thanks for the effort :). |
Closes #15708