Spacing: Optimize & condense unlinked spacing controls #50660
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +8.26 kB (+1%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
try/refactor-spacing-controls
branch
from
9155b35 to
1f31160
Compare
|
I just watched the video and got to say that this looks really good and very useful! |
aaronrobertshaw
force-pushed
the
try/refactor-spacing-controls
branch
from
1f31160 to
b33165d
Compare
|
Flaky tests detected in 34c172e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5107902130
|
aaronrobertshaw
changed the title
|
Do we want to add the new side/axis icons to the library, or should we look to create a dynamic icon component like the BoxControl? |
|
The tweak to the font size picker's header spacing is up in #50855.
The fix is really just adding a couple of styled components to the FontSizePicker to apply the required CSS styles (16px height + negative margin on the toggle). If we can create a shared component to make this layout more consistent across controls, the new styled components will be removed when the FontSizePicker is refactored to use the proposed component. I initially thought we might have gotten away with only creating the new component in the block-editor package but given the FontSizePicker lives in our components package, we'd need to add it there. It could be made private though if we don't wish to expose it. |
|
I don't personally have a strong opinion on whether we create the correct component first and then fix the spacing, or hack the spacing and then update the hacks once we have the right components. Perhaps @mirka or @jameskoster can chime in? |
| @@ -29,7 +29,7 @@ export default function SidesDropdown( { | |||
| <DropdownMenu | |||
| icon={ sideIcon } | |||
| label={ labelProp } | |||
| className="component-spacing-sizes-control__dropdown" | |||
| className="components-spacing-sizes-control__dropdown" | |||
There was a problem hiding this comment.
Just realized that we're using components-* class names for this component, even if it's not in the @wordpress/components package — this could be causing confusion around which package this component belongs to, and in a way "legitimize" using components-* classnames outside of the @wordpress/components package (which we've been advocating against for some time).
Any changes we could change the components-* class name prefix? (of course, it can happen in a separate PR)
There was a problem hiding this comment.
Good point.
In this case, it was copied from existing controls when the spacing controls were created.
There are a lot of cases where components in packages outside of @wordpress/components prefix their component classes with components- as well. These include the editor, block-editor, and block-library packages.
The water is muddied further for the spacing controls due to the styling overrides for the range control's marks, etc.
The tweak commented on here was to make the existing class name at least consistent within the same control. As the spacing sizes control has so far only been exported as experimental from the block-editor package, I think we can change its class names without too much issue.
Other uses of components- in class names will need to be evaluated separately as suggested.
There was a problem hiding this comment.
I've pushed some changes to remove the components prefix from the spacing sizes control. Unfortunately, the need to segment the range control does still require the use of some of the range control's internal classes.
There was a problem hiding this comment.
The tweak commented on here was to make the existing class name at least consistent within the same control. As the spacing sizes control has so far only been exported as experimental from the block-editor package, I think we can change its class names without too much issue.
Thank you @aaronrobertshaw for the super quick work!
There are a lot of cases where components in packages outside of
@wordpress/componentsprefix their component classes withcomponents-as well. These include theeditor,block-editor, andblock-librarypackages.
That is absolutely true. It is definitely an uphill battle, especially since only a small group of us are actively trying to bring awareness about it.
It may be hard to remove a lot of those instances outside of the components package, but the least we can do is to avoid introducing new ones and make folks more aware of the high cost that style overrides can have on maintaining our UI components. Your work here is definitely appreciated!
Other uses of
components-in class names will need to be evaluated separately as suggested.
Makes sense, let's definitely look at this separately.
Style overrides are usually added by folks when they can't achieve the desired result with the available components and their props. When trying to remove those overrides, we should ask ourselves: is the limitation that we're encountering by design? Should we consider updating the component to fulfill this new need? Or should we reconsider what we're working on and use the component how it was intended?
There was a problem hiding this comment.
When trying to remove those overrides, we should ask ourselves: is the limitation that we're encountering by design?
I'd say for the RangeControl, yes, it is by design, however, the spacing controls being segmented is a pretty left-field design/edge case.
Should we consider updating the component to fulfill this new need?
We do have the updated SliderControls on the horizon so I don't think need to update a component that should be replaced eventually.
Or should we reconsider what we're working on and use the component how it was intended?
This wouldn't be possible while meeting the design. So if we don't want the small CSS tweak to the mark positions, we're left with updating the component or creating a "SegmentedRangeControl".
Ultimately, this overriding of the range control's styles isn't introduced in this PR, it's actually lessened by it.
Any follow-ups can make a decision to updating the RangeControl etc.
There was a problem hiding this comment.
Ultimately, this overriding of the range control's styles isn't introduced in this PR, it's lessened by it.
Yup! My earlier comment referred to the broader context around this component, and not this PR specifically.
This wouldn't be possible while meeting the design.
This is very much the point — if we end up in a situation where our designs don't meet the requirements set by other previous design specs, ideally, we should either update the older specs to meet new requirements or update the new design to follow the older specs.
Not a piece of feedback specifically for this PR but more of a general reflection on how I think that, as the developers implementing a design, we should flag this kind of inconsistencies and force them to be resolved one way or the other
|
Nice work. For me, when switching to custom, the custom input is taking all the space and squashing the slider: |
|
Thanks for catching that @glendaviesnz 👍 I missed the custom value's range slider when removing as many of the internal component class names as possible. I've fixed that in 7c14c9f. It should be ready for another test now 🙏 
|
packages/block-editor/src/components/spacing-sizes-control/utils.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/spacing-sizes-control/utils.js
Outdated
Show resolved
Hide resolved
|
The only other thing I have found is the alignment is wrong in the custom height setting in the Spacer block. 
|
|
Thanks for the thorough testing @glendaviesnz 🙇
When outside the ToolsPanel, it looks like While looking into this I realised that the The BlockGap took a different approach and fudged things using the top or left sides. It turns out both resulted in incorrect, or unintended, visually hidden labels e.g. "undefined height" (since all isn't in the list of supported sides) or "top block spacing". 
I've leveraged the existing Let me know what you think and if there's anything else I missed. |
There was a problem hiding this comment.
Tested well for me:
✅ Unit tests ran successfully
✅ Was able to adjust spacing on various blocks and axis with both preset and custom values
✅ Only supported sides appear in the dropdown
✅ Setting "customSpacingSize":false in theme.json caused only custom inputs to display
✅ Setting > 8 spacing preset sizes caused select list to appear instead of slider
✅ Setting all possible combinations of supported sides in block.json worked as expected
|
🎉 |
Fixes: #49264
What?
Refactors the SpacingSizesControl to reduce the footprint of the control when its sides are unlinked.
🤖 Generated by Copilot at b33165d
This pull request improves the
SpacingSizesControlcomponent and its subcomponents in theblock-editorpackage by adding new icons, a new dropdown menu, a new hook, and a new UI. It also removes some unused or unnecessary code and props, and updates the import paths, the styles, and the tests. The overall purpose is to simplify the UI and make it easier to switch between different views of the spacing sizes.Why?
When the spacing controls are unlinked they take up a huge amount of real estate. This is compounded multiple times if the block has support for padding, margin, block gap and any other ad hoc controls that use the same component.
How?
🤖 Generated by Copilot at b33165d
splitOnAxisprop fromSpacingSizesControlcomponent and simplify UI (link,link,link)useSettinghook and default values into a new custom hookuseSpacingSizes(link,link)SingleInputControlthat renders a single input control for a single side (link)SidesDropdownthat renders a dropdown menu that allows users to switch between different views of theSpacingSizesControlcomponent (link)SpacingSizesControlcomponent and its subcomponents (link,link,link,link,link,link,link)SpacingInputControlcomponent to use the new icons and a custom toggle button to switch between preset and custom values (link,link,link,link,link,link,link,link,link)AxialInputControlsandSeparatedInputControlscomponents to use the newhasAxisSupportfunction and theiconprop (link,link,link,link,link)hasAxisSupport,getSupportedMenuItems,hasBalancedSidesSupport, andgetInitialViewto handle the new logic of theSpacingSizesControlcomponent and its subcomponents (link)SpacingSizesControlcomponent and its subcomponents according to the new UI (link,link,link,link)utils.jsfile (link,link)all-input-control.jsandlinked-button.js(link,link)Possible Next Steps
Decide upon if the "linked" or all sides view should be removed. (It still provides a handy convenience and helps reduce footprint for odd side configurations e.g. top & left etc)Linked view has been removed.Reinstate hints if its decided they should be retainedHints will be omitted for now.Sort out a few more edge cases in terms of which view options should be presented at what times.Testing Instructions
npm run test:unit packages/block-editor/src/components/spacing-sizes-control/test/utils.js> 8spacing preset sizesTesting Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2023-05-19.at.1.59.38.pm.mp4