Skip to content
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

Spacing Sizes Control: Try improving layout spacing #44858

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Oct 11, 2022

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?

  • Prevents the grid template row being too tall for the intended 16px labels
  • Remove 16px height styles squashing icon buttons
  • Apply a negative margin to icon buttons to not enlarge the intended 16px label row
  • Removed some unnecessary and duplicate styles
  • Make the range controls vertically center within the 40px height wrapper previously given
  • Upsize the custom input UnitControl and RangeControl so they are 40px as well
  • Update margins so there's an 8px gap between the label row and inputs & 16px gap between inputs and next side label

Testing Instructions

  1. Test the control in the site editor; e.g. Global Styles > Blocks > Group > Layout
  2. In the post editor, try it out for a block that has spacing support e.g. Group
  3. Inspecting via dev tools you can get a better view of what spacing has been applied. Crosscheck with Figma
  4. Update your theme.json such that your spacing presets has > 8 steps. See settings.spacing.spacingScale.steps.
  5. Confirm the custom select control has consistent spacing and height
  6. Confirm visual appearance across the usual culprits: Chrome, Safari, Firefox etc.

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

@aaronrobertshaw aaronrobertshaw added [Feature] UI Components Impacts or related to the UI component system [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Oct 11, 2022
@aaronrobertshaw aaronrobertshaw self-assigned this Oct 11, 2022
@github-actions
Copy link

github-actions bot commented Oct 11, 2022

Size Change: -5 B (0%)

Total Size: 1.27 MB

Filename Size Change
build/block-editor/index.min.js 167 kB -88 B (0%)
build/block-editor/style-rtl.css 15.4 kB +16 B (0%)
build/block-editor/style.css 15.4 kB +18 B (0%)
build/block-library/blocks/site-logo/editor-rtl.css 490 B +2 B (0%)
build/block-library/blocks/site-logo/editor.css 490 B +2 B (0%)
build/block-library/editor-rtl.css 11.2 kB +2 B (0%)
build/block-library/editor.css 11.2 kB +2 B (0%)
build/components/style-rtl.css 11.2 kB +6 B (0%)
build/components/style.css 11.2 kB +6 B (0%)
build/edit-post/index.min.js 31.8 kB -17 B (0%)
build/edit-site/index.min.js 57.8 kB +55 B (0%)
build/edit-widgets/index.min.js 16.7 kB -9 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 982 B
build/annotations/index.min.js 2.76 kB
build/api-fetch/index.min.js 2.26 kB
build/autop/index.min.js 2.14 kB
build/blob/index.min.js 475 B
build/block-directory/index.min.js 7.09 kB
build/block-directory/style-rtl.css 990 B
build/block-directory/style.css 991 B
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 84 B
build/block-library/blocks/avatar/style.css 84 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 482 B
build/block-library/blocks/button/editor.css 482 B
build/block-library/blocks/button/style-rtl.css 523 B
build/block-library/blocks/button/style.css 523 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 100 B
build/block-library/blocks/categories/style.css 100 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 187 B
build/block-library/blocks/comment-template/style.css 185 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 834 B
build/block-library/blocks/comments/editor.css 832 B
build/block-library/blocks/comments/style-rtl.css 632 B
build/block-library/blocks/comments/style.css 630 B
build/block-library/blocks/cover/editor-rtl.css 612 B
build/block-library/blocks/cover/editor.css 613 B
build/block-library/blocks/cover/style-rtl.css 1.57 kB
build/block-library/blocks/cover/style.css 1.55 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 346 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 948 B
build/block-library/blocks/gallery/editor.css 950 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 394 B
build/block-library/blocks/group/editor.css 394 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 327 B
build/block-library/blocks/html/editor.css 329 B
build/block-library/blocks/image/editor-rtl.css 884 B
build/block-library/blocks/image/editor.css 882 B
build/block-library/blocks/image/style-rtl.css 627 B
build/block-library/blocks/image/style.css 630 B
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 463 B
build/block-library/blocks/latest-posts/style.css 462 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 705 B
build/block-library/blocks/navigation-link/editor.css 703 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation/editor-rtl.css 2.02 kB
build/block-library/blocks/navigation/editor.css 2.03 kB
build/block-library/blocks/navigation/style-rtl.css 2.17 kB
build/block-library/blocks/navigation/style.css 2.16 kB
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 443 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 363 B
build/block-library/blocks/page-list/editor.css 363 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 317 B
build/block-library/blocks/paragraph/editor.css 317 B
build/block-library/blocks/paragraph/style-rtl.css 279 B
build/block-library/blocks/paragraph/style.css 281 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 493 B
build/block-library/blocks/post-comments-form/style.css 493 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 586 B
build/block-library/blocks/post-featured-image/editor.css 584 B
build/block-library/blocks/post-featured-image/style-rtl.css 315 B
build/block-library/blocks/post-featured-image/style.css 315 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 282 B
build/block-library/blocks/post-template/style.css 282 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 326 B
build/block-library/blocks/pullquote/style.css 325 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 282 B
build/block-library/blocks/query-pagination/style.css 278 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 439 B
build/block-library/blocks/query/editor.css 439 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 409 B
build/block-library/blocks/search/style.css 406 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 464 B
build/block-library/blocks/shortcode/editor.css 464 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 322 B
build/block-library/blocks/spacer/editor.css 322 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 494 B
build/block-library/blocks/table/editor.css 494 B
build/block-library/blocks/table/style-rtl.css 611 B
build/block-library/blocks/table/style.css 609 B
build/block-library/blocks/table/theme-rtl.css 190 B
build/block-library/blocks/table/theme.css 190 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 235 B
build/block-library/blocks/template-part/editor.css 235 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 691 B
build/block-library/blocks/video/editor.css 694 B
build/block-library/blocks/video/style-rtl.css 174 B
build/block-library/blocks/video/style.css 174 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 162 B
build/block-library/classic.css 162 B
build/block-library/common-rtl.css 1.02 kB
build/block-library/common.css 1.02 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 192 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 12.3 kB
build/block-library/style.css 12.3 kB
build/block-library/theme-rtl.css 719 B
build/block-library/theme.css 722 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 49.8 kB
build/components/index.min.js 202 kB
build/compose/index.min.js 12.5 kB
build/core-data/index.min.js 15.5 kB
build/customize-widgets/index.min.js 11.3 kB
build/customize-widgets/style-rtl.css 1.38 kB
build/customize-widgets/style.css 1.38 kB
build/data-controls/index.min.js 653 B
build/data/index.min.js 8.08 kB
build/date/index.min.js 32.1 kB
build/deprecated/index.min.js 507 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.7 kB
build/edit-navigation/index.min.js 16.1 kB
build/edit-navigation/style-rtl.css 3.99 kB
build/edit-navigation/style.css 4 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/style-rtl.css 7.13 kB
build/edit-post/style.css 7.13 kB
build/edit-site/style-rtl.css 8.36 kB
build/edit-site/style.css 8.35 kB
build/edit-widgets/style-rtl.css 4.34 kB
build/edit-widgets/style.css 4.34 kB
build/editor/index.min.js 41.6 kB
build/editor/style-rtl.css 3.62 kB
build/editor/style.css 3.61 kB
build/element/index.min.js 4.68 kB
build/escape-html/index.min.js 537 B
build/experiments/index.min.js 868 B
build/format-library/index.min.js 6.95 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.64 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.77 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.78 kB
build/keycodes/index.min.js 1.83 kB
build/list-reusable-blocks/index.min.js 2.13 kB
build/list-reusable-blocks/style-rtl.css 835 B
build/list-reusable-blocks/style.css 835 B
build/media-utils/index.min.js 2.93 kB
build/notices/index.min.js 963 B
build/nux/index.min.js 2.06 kB
build/nux/style-rtl.css 732 B
build/nux/style.css 728 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.22 kB
build/preferences/index.min.js 1.33 kB
build/primitives/index.min.js 933 B
build/priority-queue/index.min.js 1.58 kB
build/react-i18n/index.min.js 696 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.74 kB
build/reusable-blocks/index.min.js 2.21 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.6 kB
build/server-side-render/index.min.js 1.77 kB
build/shortcode/index.min.js 1.53 kB
build/style-engine/index.min.js 1.46 kB
build/token-list/index.min.js 644 B
build/url/index.min.js 3.61 kB
build/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 268 B
build/widgets/index.min.js 7.21 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.19 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@jasmussen
Copy link
Contributor

Looks good, no longer looks skewed, and there's a gutter between slider and input!

Screenshot 2022-10-11 at 08 53 53

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!

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love a check by @jameskoster as well, but giving a preliminary apprpoval.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the quick review @jasmussen, the paint was barely dry on it 🚀

but it would be good to have a 16px gutter between input and slider

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.

Would love a check by @jameskoster as well, but giving a preliminary approval.

I'll keep an eye out for any feedback @jameskoster has and will iron that out tomorrow.

@jasmussen
Copy link
Contributor

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! 😅

margin-top: 8px;
}

.components-range-control {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, thank you!

@ciampo
Copy link
Contributor

ciampo commented Oct 11, 2022

We can address any other UnitControl/RangeControl combos as follow-ups.

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

@aaronrobertshaw
Copy link
Contributor Author

I've just pushed an additional tweak to ensure the custom select control also has a consistent margin and height. Setting the spacingScale.steps in theme.json to 10 is an easy way to trigger this view in the component to test.

Screen Shot 2022-10-11 at 8 16 21 pm

Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better :)

Before:

before.mp4

After:

after.mp4

@glendaviesnz glendaviesnz added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 11, 2022
@glendaviesnz
Copy link
Contributor

glendaviesnz commented Oct 11, 2022

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?

@aaronrobertshaw
Copy link
Contributor Author

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?

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

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?

@jasmussen
Copy link
Contributor

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.

@aaronrobertshaw
Copy link
Contributor Author

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 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 Tooltip and its popovers. This was likely to make positioning them easier.

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 Tooltip so we can enforce they stay within the component's container or the viewport etc. The work there is ongoing but a little lower down the current list of priorities.

As mentioned above, this doesn't have to be a blocker for these improvements.

Though I would consider that bugfix territory, and seems trivial

It "seems" that way 😄

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@glendaviesnz glendaviesnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sizing works correctly now for me between preset and custom sizing

@aaronrobertshaw aaronrobertshaw merged commit d7e9b29 into trunk Oct 13, 2022
@aaronrobertshaw aaronrobertshaw deleted the try/improving-spacing-sizes-control-layout branch October 13, 2022 04:23
@github-actions github-actions bot added this to the Gutenberg 14.4 milestone Oct 13, 2022
@ciampo
Copy link
Contributor

ciampo commented Oct 13, 2022

Is it too late to backport this for 6.1 ? (edit: just seen the "backport" label, nevermind)

@michalczaplinski
Copy link
Contributor

I just cherry-picked this PR to the wp/6.1-rc-2 branch to get it included in the next release: 346ff74

@michalczaplinski michalczaplinski removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label 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
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Feature] UI Components Impacts or related to the UI component system Needs User Documentation Needs new user documentation [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

8 participants