Spacing Sizes Control: Try improving layout spacing #44858
Merged
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
aaronrobertshaw
added
[Feature] UI Components
aaronrobertshaw
requested review from
jameskoster,
jasmussen and
glendaviesnz
|
Size Change: -5 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
Looks good, no longer looks skewed, and there's a gutter between slider and input!
Very probably separate to this particular PR, so not a blocker, but it would be good to have a 16px gutter between input and slider. It's okay that the knob "overlaps" the gutter, as it sits on the far left of the slider. But in a knobless slider the gap between should be 16px. As noted, probably separate and part of 43423. Thank you! |
jasmussen
approved these changes
Oct 11, 2022
There was a problem hiding this comment.
Would love a check by @jameskoster as well, but giving a preliminary apprpoval.
|
Thanks for the quick review @jasmussen, the paint was barely dry on it 🚀
The styles for this component set the margin providing the gap and I already tweaked them once to be more consistent with other controls like this. I've just pushed a tweak to update these to have the 16px gap. We can address any other UnitControl/RangeControl combos as follow-ups.
I'll keep an eye out for any feedback @jameskoster has and will iron that out tomorrow. |
|
I think you can land this when the tests pass, if it has to go quick, honestly. But if it isn't urgent, there's time for brewing the morning coffee! 😅 |
ciampo
reviewed
Oct 11, 2022
| margin-top: 8px; | ||
| } | ||
|
|
||
| .components-range-control { |
There was a problem hiding this comment.
Any reasons why we're using the internal classname (components-range-control) instead of the classname assigned specifically by SpacingSizesControl (i.e. components-spacing-sizes-control__custom-value-range) ?
There was a problem hiding this comment.
Mainly just that it was the only common classname across all range controls within the SpacingSizesControl. The classname you mentioned is only used for the custom spacing values. There's also the preset slider.
I can replace the components-range-control with multiple selectors.
There was a problem hiding this comment.
While taking a look at switching this out, I noticed there are already other uses of internal RangeControl class names to be able to tweak the marks etc into the design for this control. I'll hold off on replacing this for now.
There was a problem hiding this comment.
Makes sense! I guess at most we can add a note (in code or anywhere we track the follow-up work) regarding the refactor of the components-range-control classname.
There was a problem hiding this comment.
The same sort of tidy-up will need to happen for the BorderRadiusControl as well.
I took the liberty of adding this to #40507 where the replacement of the RangeControl with new slider components would happen. Makes sense to me that we refactor or clean this up only once. Of course, if that is too far out we could move that to a different issue.
There was a problem hiding this comment.
Sounds good to me, thank you!
Sounds good to me — I'd love to have a detailed assessment of any discrepancies / gaps between the design specs and the current implementations for these components |
|
I've just pushed an additional tweak to ensure the custom select control also has a consistent margin and height. Setting the
|
jameskoster
approved these changes
Oct 11, 2022
glendaviesnz
added
[Type] Bug
|
Thanks for looking at this @aaronrobertshaw. Ideally, we need to backport this to 6.1 as currently, this is a UI bug in the 6.1 RC1 release ... but is it ok for the 40px height unit controls to go in the 6.1 release as they will be different to the ones in the likes of the border control? |
That's a good question. My understanding, at least for Gutenberg, was that it was ok to update these panel by panel. Whether that still applies for the WordPress 6.1 release, I'm not sure. While the border controls would still be the shorter height, until #41860 lands and makes its way into core, the typography and color controls are already 40px tall. @jasmussen or @jameskoster would you be able to confirm that introducing the 40px high spacing controls is ok for 6.1? |
There was a problem hiding this comment.
Just took this for a quick spin, and it's looking really nice!
Probably not a blocker, since this also appears to be the case on trunk, but if I set my browser to display scrollbars (to try to mimic how things might work on Windows), in the 100% position, the tooltip on the thumb appears to introduce a horizontal scrollbar:
I was wondering if there needs to be a wider gutter on the right hand side of the slider to account for this, or would that mess up how the bar looks in relation to right aligned items near it?
|
We'll want 40px almost everywhere, and it's already present in some cases not others. It'd be nice to get them all to 40px soon (give or take some edgecases we're thinking of), but of course that's a journey. Since we're already mixing, seems okay to continue. It would be nice to understand the horizontal scrollbar, though, I missed that, sorry. Though I would consider that bugfix territory, and seems trivial. |
I believe this issue has come up before. It is due to the way the RangeControl renders its tooltips. They are a custom component, as in they don't render via The custom tooltips get absolutely positioned and I'm not sure we can rely on a known width for the tooltip contents. The result being, even if we added a gutter to the right the tooltip width could still just be long enough to extend beyond the bounds of the component forcing scrollbars. One of the items on the list for the new SliderControl component is to replace these custom tooltips with a regular As mentioned above, this doesn't have to be a blocker for these improvements.
It "seems" that way 😄 |
andrewserong
approved these changes
Oct 13, 2022
There was a problem hiding this comment.
That all makes sense to me, thanks for following-up, @aaronrobertshaw. And once this PR lands, we can always try out updates that aren't targeted to 6.1, if need be, too 👍
Just took this for another spin, and it's all testing well and looking good to me:
✅ Spacing / styling looks good in post and site editors with steps set to 7 or lower
✅ Spacing / styling looks good in post and site editors with steps set to > 7, with spacing looking consistent in the select dropdowns, and the custom font size views.
LGTM!
glendaviesnz
approved these changes
Oct 13, 2022
aaronrobertshaw
deleted the
try/improving-spacing-sizes-control-layout
branch
|
Is it too late to backport this for 6.1 ? (edit: just seen the "backport" label, nevermind) |
michalczaplinski
pushed a commit
that referenced
this pull request
Oct 18, 2022
|
I just cherry-picked this PR to the wp/6.1-rc-2 branch to get it included in the next release: 346ff74 |
michalczaplinski
removed
the
Backport to WP 6.6 Beta/RC
ockham
pushed a commit
that referenced
this pull request
Oct 18, 2022
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Oct 18, 2022
Package updates for bug and regression fixes: - `@wordpress/block-directory: 3.15.8` - `@wordpress/block-editor: 10.0.7` - `@wordpress/block-library: 7.14.8` - `@wordpress/components: 21.0.6` - `@wordpress/customize-widgets: 3.14.8` - `@wordpress/edit-post: 6.14.8` - `@wordpress/edit-site: 4.14.10` - `@wordpress/edit-widgets: 4.14.8` - `@wordpress/editor: 12.16.7` - `@wordpress/format-library: 3.15.7` - `@wordpress/interface: 4.16.6` - `@wordpress/list-reusable-blocks: 3.15.6` - `@wordpress/nux: 5.15.6` - `@wordpress/preferences: 2.9.6` - `@wordpress/reusable-blocks: 3.15.7` - `@wordpress/server-side-render: 3.15.6` - `@wordpress/widgets: 2.15.7` Original PRs from Gutenberg repository: - [WordPress/gutenberg#45041 #45041 Font Size Picker Hint: Fallback to font size `slug` if `name` is undefined] - [WordPress/gutenberg#45045 #45045 Add: Missing output escaping on some blocks] - [WordPress/gutenberg#44999 #44999 Escape comment author URL] - [WordPress/gutenberg#44972 #44972 Navigator: restore focus only once per location] - [WordPress/gutenberg#44858 #44858 Spacing Sizes Control: Try improving layout spacing] - [WordPress/gutenberg#44878 #44878 Fix: Inspector is usable on the top level block even if it is content locked] - [WordPress/gutenberg#44809 #44809 Fix list outdents on Enter in quote block] - [WordPress/gutenberg#44864 #44864 List v2: fix selection when creating paragraph from empty list item] - [WordPress/gutenberg#44853 #44853 Fix overflowing patterns] - [WordPress/gutenberg#45050 #45050 Fix visibility of nested Group block appender] - [WordPress/gutenberg#44887 #44887 wp-env: Use case insensitive regex when checking WP version string] Follow-up to [54257], [54335], [54383], [54483], [54486], [54490]. Props bernhard-reiter, audrasjb. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54632 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Oct 18, 2022
Package updates for bug and regression fixes: - `@wordpress/block-directory: 3.15.8` - `@wordpress/block-editor: 10.0.7` - `@wordpress/block-library: 7.14.8` - `@wordpress/components: 21.0.6` - `@wordpress/customize-widgets: 3.14.8` - `@wordpress/edit-post: 6.14.8` - `@wordpress/edit-site: 4.14.10` - `@wordpress/edit-widgets: 4.14.8` - `@wordpress/editor: 12.16.7` - `@wordpress/format-library: 3.15.7` - `@wordpress/interface: 4.16.6` - `@wordpress/list-reusable-blocks: 3.15.6` - `@wordpress/nux: 5.15.6` - `@wordpress/preferences: 2.9.6` - `@wordpress/reusable-blocks: 3.15.7` - `@wordpress/server-side-render: 3.15.6` - `@wordpress/widgets: 2.15.7` Original PRs from Gutenberg repository: - [WordPress/gutenberg#45041 #45041 Font Size Picker Hint: Fallback to font size `slug` if `name` is undefined] - [WordPress/gutenberg#45045 #45045 Add: Missing output escaping on some blocks] - [WordPress/gutenberg#44999 #44999 Escape comment author URL] - [WordPress/gutenberg#44972 #44972 Navigator: restore focus only once per location] - [WordPress/gutenberg#44858 #44858 Spacing Sizes Control: Try improving layout spacing] - [WordPress/gutenberg#44878 #44878 Fix: Inspector is usable on the top level block even if it is content locked] - [WordPress/gutenberg#44809 #44809 Fix list outdents on Enter in quote block] - [WordPress/gutenberg#44864 #44864 List v2: fix selection when creating paragraph from empty list item] - [WordPress/gutenberg#44853 #44853 Fix overflowing patterns] - [WordPress/gutenberg#45050 #45050 Fix visibility of nested Group block appender] - [WordPress/gutenberg#44887 #44887 wp-env: Use case insensitive regex when checking WP version string] Follow-up to [54257], [54335], [54383], [54483], [54486], [54490]. Props bernhard-reiter, audrasjb. See #56467. Built from https://develop.svn.wordpress.org/trunk@54632 git-svn-id: http://core.svn.wordpress.org/trunk@54184 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot
pushed a commit
to platformsh/wordpress-performance
that referenced
this pull request
Oct 18, 2022
Package updates for bug and regression fixes: - `@wordpress/block-directory: 3.15.8` - `@wordpress/block-editor: 10.0.7` - `@wordpress/block-library: 7.14.8` - `@wordpress/components: 21.0.6` - `@wordpress/customize-widgets: 3.14.8` - `@wordpress/edit-post: 6.14.8` - `@wordpress/edit-site: 4.14.10` - `@wordpress/edit-widgets: 4.14.8` - `@wordpress/editor: 12.16.7` - `@wordpress/format-library: 3.15.7` - `@wordpress/interface: 4.16.6` - `@wordpress/list-reusable-blocks: 3.15.6` - `@wordpress/nux: 5.15.6` - `@wordpress/preferences: 2.9.6` - `@wordpress/reusable-blocks: 3.15.7` - `@wordpress/server-side-render: 3.15.6` - `@wordpress/widgets: 2.15.7` Original PRs from Gutenberg repository: - [WordPress/gutenberg#45041 #45041 Font Size Picker Hint: Fallback to font size `slug` if `name` is undefined] - [WordPress/gutenberg#45045 #45045 Add: Missing output escaping on some blocks] - [WordPress/gutenberg#44999 #44999 Escape comment author URL] - [WordPress/gutenberg#44972 #44972 Navigator: restore focus only once per location] - [WordPress/gutenberg#44858 #44858 Spacing Sizes Control: Try improving layout spacing] - [WordPress/gutenberg#44878 #44878 Fix: Inspector is usable on the top level block even if it is content locked] - [WordPress/gutenberg#44809 #44809 Fix list outdents on Enter in quote block] - [WordPress/gutenberg#44864 #44864 List v2: fix selection when creating paragraph from empty list item] - [WordPress/gutenberg#44853 #44853 Fix overflowing patterns] - [WordPress/gutenberg#45050 #45050 Fix visibility of nested Group block appender] - [WordPress/gutenberg#44887 #44887 wp-env: Use case insensitive regex when checking WP version string] Follow-up to [54257], [54335], [54383], [54483], [54486], [54490]. Props bernhard-reiter, audrasjb. See #56467. Built from https://develop.svn.wordpress.org/trunk@54632 git-svn-id: https://core.svn.wordpress.org/trunk@54184 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ootwch
pushed a commit
to ootwch/wordpress-develop
that referenced
this pull request
Nov 4, 2022
Package updates for bug and regression fixes: - `@wordpress/block-directory: 3.15.8` - `@wordpress/block-editor: 10.0.7` - `@wordpress/block-library: 7.14.8` - `@wordpress/components: 21.0.6` - `@wordpress/customize-widgets: 3.14.8` - `@wordpress/edit-post: 6.14.8` - `@wordpress/edit-site: 4.14.10` - `@wordpress/edit-widgets: 4.14.8` - `@wordpress/editor: 12.16.7` - `@wordpress/format-library: 3.15.7` - `@wordpress/interface: 4.16.6` - `@wordpress/list-reusable-blocks: 3.15.6` - `@wordpress/nux: 5.15.6` - `@wordpress/preferences: 2.9.6` - `@wordpress/reusable-blocks: 3.15.7` - `@wordpress/server-side-render: 3.15.6` - `@wordpress/widgets: 2.15.7` Original PRs from Gutenberg repository: - [WordPress/gutenberg#45041 #45041 Font Size Picker Hint: Fallback to font size `slug` if `name` is undefined] - [WordPress/gutenberg#45045 #45045 Add: Missing output escaping on some blocks] - [WordPress/gutenberg#44999 #44999 Escape comment author URL] - [WordPress/gutenberg#44972 #44972 Navigator: restore focus only once per location] - [WordPress/gutenberg#44858 #44858 Spacing Sizes Control: Try improving layout spacing] - [WordPress/gutenberg#44878 #44878 Fix: Inspector is usable on the top level block even if it is content locked] - [WordPress/gutenberg#44809 #44809 Fix list outdents on Enter in quote block] - [WordPress/gutenberg#44864 #44864 List v2: fix selection when creating paragraph from empty list item] - [WordPress/gutenberg#44853 #44853 Fix overflowing patterns] - [WordPress/gutenberg#45050 #45050 Fix visibility of nested Group block appender] - [WordPress/gutenberg#44887 #44887 wp-env: Use case insensitive regex when checking WP version string] Follow-up to [54257], [54335], [54383], [54483], [54486], [54490]. Props bernhard-reiter, audrasjb. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54632 602fd350-edb4-49c9-b593-d223f7449a82
16 tasks
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.
What?
Fixes a few spacing and size issues within the spacing sizes control. It also upsizes the inputs to 40px while we have the chance (also made it easier to address some of the spacing tweaks).
Why?
We all want a clean, tidy and aligned UI.
How?
Testing Instructions
> 8steps. Seesettings.spacing.spacingScale.steps.Note: There as some horizontal scrolling issues with the sidebar in Firefox and Safari but they occur on trunk and look unrelated to these changes
Screenshots or screencast
Screen.Recording.2022-10-11.at.3.51.05.pm.mp4