UnitControl: Normalize wrapper classnames by removing outer wrapper #45139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is achieved by removing the unnecessary outer wrapper for the UnitControl.
aaronrobertshaw
added
[Type] Enhancement
|
|
aaronrobertshaw
commented
Oct 20, 2022
There was a problem hiding this comment.
Before removing the wrapper, I went through a bunch of searches in Gutenberg looking for anything targeting or relying on the outer wrapper.
The only direct reference found was in the border radius control. So I made this PR based off #41860 where we remove that and introduce the __unstableWrapperClassname to the UnitControl. I figure we could merge this back into #41860 to avoid that addition even momentarily.
The BorderControl also leveraged the styled div Root component from the UnitControl. Switching that to the styled input worked smoothly.
There were a few places passing inline styles to the UnitControl which were previously placed on the outer wrapper. They are still on the outer most element and I didn't spot any issues for these in testing. One example of this was the spacing presets control.
|
Size Change: -41 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
@glendaviesnz if you have time would you mind giving this one a double-check for the spacing sizes control? I'd like to make sure it doesn't mess anything up there for you. |
ciampo
reviewed
Oct 20, 2022
There was a problem hiding this comment.
Thank you for working on this so quickly, @aaronrobertshaw !
I figure we could merge this back into #41860 to avoid that addition even momentarily.
Sounds like a good idea, especially since this PR doesn't look that large anyway.
The BorderControl also leveraged the styled div Root component from the UnitControl. Switching that to the styled input worked smoothly.
Nice!
There were a few places passing inline styles to the UnitControl which were previously placed on the outer wrapper. They are still on the outer most element and I didn't spot any issues for these in testing.
Good point. That would be my expectation too, but we should definitely do some extensive testing
Doesn't appear to affect the spacing size controls at all. |
aaronrobertshaw
added a commit
that referenced
this pull request
Oct 24, 2022
aaronrobertshaw
added a commit
that referenced
this pull request
Oct 26, 2022
ciampo
added a commit
that referenced
this pull request
Oct 26, 2022
* Update border control components to allow 40px height * Make 40px height work again after rebase * Turn on 40px tall border controls in editors * Update radius control to handle extra padding These tweaks update the BorderRadiusControl such that the entered values are actually visible despite the increased internal padding applied to the UnitControl when the `__unstable-large` variant is used. * First pass at improving border controls when at 40px Initial tweaks to improve the UX for the border controls. Aims to address the increased padding for large 40px high UnitControl variant that cuts of entered values from display. * Make compact inputs 118px wide * Increase padding to 16px between input and slider * Make split controls gap a uniform 16px * Update split layout for floating linked button * Make color dropdown 40px square with 16px indicator * Tweak input widths again to fit editor sidebar * Make radius control match border and grid * Remove hardcoded width from radius input wrapper * Try using Mix as placeholder for mixed values * Revert placeholder back to 'Mixed' * Remove use of internal component classnames * Remove unnecessary flex styles from linked button * Use space util for color swatch dimensions * Add explanatory comment for compact widths * Better align the left border for 30px height variant * Consolidate border radius controls to single 40px height This allows us to remove most of the changes that would have been introduced in this PR as well as simplify the CSS. * Fix RTL rendering and style naming * Remove unneeded prop setting 40px height * Switch __next40pxDefaultSize to size __unstable-large * Fix up defaults in types and update READMEs * Add components changelog * Rename new prop `__unstableWrapperClassName` * UnitControl: Normalize wrapper classnames by removing outer wrapper (#45139) * BorderBoxControl: Refactor stories to TypeScript and Controls (#45002) * Remove max-width wrappers from border control stories Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
|
Looks like there were a couple of blocks that force a max-width on bespoke controls via inline styles. These include Cover, Search, and Spacer blocks. A fix has been put up in #45329. |
This was referenced Oct 27, 2022
ciampo
added
the
has dev note
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #45094
Related:
What?
Normalizes the application of class names for the UnitControl, so it is applied to the outermost wrapper.
Why?
classNameprops being applied to the outermost element.How?
This PR removes the outer wrapper for the UnitControl. It appears unnecessary, and with only a few other minor tweaks, we maintain the status quo in terms of layout for other components e.g. BorderRadiusControl, while also bringing UnitControl in line with our other components.
Testing Instructions
components-unit-control-wrapperclass that was formerly on the outer wrapper.✍️ Dev Note
In order to improve consistency across the
@wordpress/componentspackage, any class names to theUnitControlcomponent through itsclassNameprop will be applied to its outer wrapper element, instead of an inner input wrapper element.As a consequence of this change, the
components-unit-control-wrapperclass name and the__unstableWrapperClassNameprop have been removed from the component.