WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Components: Some components are missing `box-sizing` reset styles #42415

Open mirka opened 2 years ago

mirka commented 2 years ago

What problem does this address?

42294 surfaced two problems:

What is your proposed solution?

I think we're going to need to start checking that each component includes box-sizing resets for their own HTML subtree. The approach would be something like:


This is not necessarily meant as a "project" where we need to clean up every component in the package, but something that we should start cleaning up in a consistent way whenever we notice box-sizing issues in a given component.

ciampo commented 2 years ago

I'm also not exactly sure about how this issue hasn't been flagged before 🤯

Components with SCSS files can use the reset mixin. Add a reusable reset mixin equivalent for the Emotion components. (But give it a more explicit name like boxSizingReset.)

Sounds good. In SCSS files, I would also add a comment pointing at that Emotion equivalent mixin for when the component is refactored to CSS-in-JS ?

Maybe add it to the View component?

Sounds good on paper — in practice it may need a bit more investigation, in case this change causes any regressions.

Once a component has its box-sizing reset in place, the ad hoc box-sizing styles can be removed.

Sounds good

This is not necessarily meant as a "project" where we need to clean up every component in the package, but something that we should start cleaning up in a consistent way whenever we notice box-sizing issues in a given component.

Not sure about this — I'm a big fan of consistency, and in a way I like the idea that a developer could assume that all components from the library have box-sizing: border-box set. This would also avoid scenarios in which the box-sizing of a component will depend on the context in which it is rendered (in case of style leaks).

I'm sure that applying these styles may cause a few regressions, but I would argue that it's better to find and fix regressions caused "willingly", than keeping the styles as they are currently and incur in issues similar to #42294 every once in a whilte

mirka commented 2 years ago

We might consider adding a single "component reset" mixin that we include in the top-level wrapper of each component, which would include the border sizing reset, font-size/line-height, color, etc.

mirka commented 2 years ago

Not sure about this — I'm a big fan of consistency, and in a way I like the idea that a developer could assume that all components from the library have box-sizing: border-box set. This would also avoid scenarios in which the box-sizing of a component will depend on the context in which it is rendered (in case of style leaks).

Yes, I agree this is the ideal state. It's more a matter of triage that I suggested not doing a package-wide fix, given the amount of effort it would take to apply the reset to every single component. The good news is that, box-sizing problems are easier to notice now with the CSS switcher in Storybook (#42747) — the default preset doesn't have any box-sizing resets.

ciampo commented 2 years ago

We might consider adding a single "component reset" mixin that we include in the top-level wrapper of each component, which would include the border sizing reset, font-size/line-height, color, etc.

This sounds like a good idea!

Yes, I agree this is the ideal state. It's more a matter of triage that I suggested not doing a package-wide fix, given the amount of effort it would take to apply the reset to every single component. The good news is that, box-sizing problems are easier to notice now with the CSS switcher in Storybook (#42747) — the default preset doesn't have any box-sizing resets.

Sounds good. I guess we can work our way gradually (in order to avoid the risks of a one-shot, package-wide fix), with the ultimate aim of having all components "box-sized" equally