-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Adds box-sizing: border-box; as a wildcard to base.css. Removes box-s… #529
Adds box-sizing: border-box; as a wildcard to base.css. Removes box-s… #529
Conversation
…izing from buttons-core.css, forms.css, and menus-core.css.
Can this be an opt-in? Only activated with a certain |
@lkraav I recommend not having an opt-in option because of the radical box-model differences you'd experience without a consistently applied box-sizing attribute. This would especially affect forms, buttons, and any elements whose width switches to 100% on a responsive breakpoint. |
Well, sure, but that's what it is right now. I'm all for increasing consistency, but there should be an option to either opt-in or out, especially since it'd be quite easily achieved. |
@lkraav Yes, creating an option is indeed easy, but my point is that if I choose to opt-in and you choose to opt-out, then we'd experience very different results from using the same modules. Omitting end user choice preserves the end expectations no matter who implements this. |
I guess I can always override this in my own CSS initialization. |
@lkraav I found a better way to implement that allows for module-specific overrides. Will update now. https://css-tricks.com/inheriting-box-sizing-probably-slightly-better-best-practice/ |
…he universal selector, allowing for module-specific box-sizing overrides.
Regarding the failed CI check, it's because I used the universal selector, which is marked as too slow performing. While that is correct, box-sizing is one area when it's appropriate to use the universal selector. |
I don't think it's an issue giving someone an option. I don't see this as any different than having an option for a fluid/full width container vs a responsive container with a max width. It's an option. Another problem, this is not backwards compatible. If someone updates, it automatically gets applied and can mess things up. I believe that unless it's a major release, there should not be major changes that need to be made by the users. |
@mattconvente agree with @jvincilione. I don't see this getting in unless there is a major version release so that the risk of breaking existing users is mitigated. I set the milestone of this to 1.0 so its to track for later inclusion. |
CLA is valid! |
Is this still live or what? |
@mattconvente while we are still active is it possible to address #735 ? |
@DonaldTsang I added my now closed PR years ago as part of Digital Ocean's 2015 Hacktoberfest. I only closed it because your comment led me to see it was still open, and the previous comments showed my code would introduce a large breaking change. As such, I wouldn't really consider me still active, and I won't be contributing further. |
@mattconvente who manages this repo then? |
@DonaldTsang I'm unsure who currently manages this repo, or if it's managed at all. I contributed to Hacktoberfest for a tshirt. I'm by no means a frequent OSS contributor, and quite honestly, until your comment I had forgotten I even made this PR. Again, as such, I don't plan to contribute further, so please seek answers elsewhere. |
@redonkulus manages this repo. |
We recently released v2.0.0 but did not include this change. If anyone wants to test this out we could pull this in since the release is still new. |
…izing from buttons-core.css, forms.css, and menus-core.css.