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

[Components] Update FontSize control #35395

Merged
merged 13 commits into from
Oct 14, 2021
Merged

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Oct 6, 2021

This PR is part of Typography tool update: #34345 and is focuses on the FontSizePicker control.

The purpose of this PR is to try to see what is missing and how we will move forward to match the current design and also maybe revisit them to take into account our actual needs. We should make this iteratively and decide on these iterations. Also don't mind the actual code too much, as is mostly exploratory and will need refactoring.

Screenshots

Current design

Screen Shot 2021-09-24 at 2 00 42 PM

Predef (< 6 sizes) Predef (>= 6 sizes) Custom font size
image image image

And also from this: #34345 (comment)

Screenshot 2021-10-06 at 1 10 50 PM

These first explorations have already raised some issues and some of them are things from changes I've not committed here.

For a first iteration we could probably skip the dual control, but we'll see 😄 :

When picking a font size from a list of predefined sizes, show either a ToggleGroup control (< 6 sizes) or a dropdown control (>= 6 sizes) (see screenshots below and this early WIP prototype)

Notes

Control Label

We need to decide what should be shown next to the Size label in all cases (custom, from existing option, default, etc..)
Screenshot 2021-10-06 at 1 15 51 PM

Custom Select component

Does changes in this components sound good like I've done in this PR? It would need better css handling though if we keep these..
Also the design is a bit different with the mock up as there is no space above the showed options.

Range Control

Currently the control supports only unitless values. We need to make this work with units. We would also need 'position handling' support as it currently shows the slider on the left.

Testing instructions

  1. Just add a paragraph and check the font size control.

@ntsekouras ntsekouras added [Package] Components /packages/components [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Oct 6, 2021
@ntsekouras ntsekouras self-assigned this Oct 6, 2021
@github-actions
Copy link

github-actions bot commented Oct 6, 2021

Size Change: +598 B (0%)

Total Size: 1.07 MB

Filename Size Change
build/block-library/blocks/social-link/editor-rtl.css 177 B +12 B (+7%) 🔍
build/block-library/blocks/social-link/editor.css 177 B +12 B (+7%) 🔍
build/block-library/blocks/social-links/editor-rtl.css 824 B +12 B (+1%)
build/block-library/blocks/social-links/editor.css 823 B +12 B (+1%)
build/block-library/blocks/social-links/style-rtl.css 1.32 kB +14 B (+1%)
build/block-library/blocks/social-links/style.css 1.32 kB +14 B (+1%)
build/block-library/editor-rtl.css 9.8 kB +12 B (0%)
build/block-library/editor.css 9.8 kB +14 B (0%)
build/block-library/style-rtl.css 10.4 kB +14 B (0%)
build/block-library/style.css 10.4 kB +14 B (0%)
build/components/index.min.js 217 kB +285 B (0%)
build/components/style-rtl.css 15.3 kB +79 B (+1%)
build/components/style.css 15.3 kB +79 B (+1%)
build/edit-site/index.min.js 29.8 kB +25 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 931 B
build/admin-manifest/index.min.js 1.09 kB
build/annotations/index.min.js 2.7 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.08 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.2 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/index.min.js 134 kB
build/block-editor/style-rtl.css 13.9 kB
build/block-editor/style.css 13.9 kB
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 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 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 474 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 600 B
build/block-library/blocks/button/style.css 600 B
build/block-library/blocks/buttons/editor-rtl.css 315 B
build/block-library/blocks/buttons/editor.css 315 B
build/block-library/blocks/buttons/style-rtl.css 370 B
build/block-library/blocks/buttons/style.css 370 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 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 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 206 B
build/block-library/blocks/columns/editor.css 205 B
build/block-library/blocks/columns/style-rtl.css 497 B
build/block-library/blocks/columns/style.css 496 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.17 kB
build/block-library/blocks/cover/style.css 1.17 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 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 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 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 977 B
build/block-library/blocks/gallery/editor.css 982 B
build/block-library/blocks/gallery/style-rtl.css 1.6 kB
build/block-library/blocks/gallery/style.css 1.59 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 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 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 502 B
build/block-library/blocks/image/style.css 505 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 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 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 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 493 B
build/block-library/blocks/media-text/style.css 490 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 568 B
build/block-library/blocks/navigation-link/editor.css 570 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 300 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/style-rtl.css 195 B
build/block-library/blocks/navigation-submenu/style.css 195 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.71 kB
build/block-library/blocks/navigation/editor.css 1.71 kB
build/block-library/blocks/navigation/style-rtl.css 1.64 kB
build/block-library/blocks/navigation/style.css 1.64 kB
build/block-library/blocks/navigation/view.min.js 2.74 kB
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 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 198 B
build/block-library/blocks/page-list/style.css 198 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/editor-rtl.css 210 B
build/block-library/blocks/post-author/editor.css 210 B
build/block-library/blocks/post-author/style-rtl.css 182 B
build/block-library/blocks/post-author/style.css 181 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B
build/block-library/blocks/post-comments-form/style.css 140 B
build/block-library/blocks/post-comments/style-rtl.css 360 B
build/block-library/blocks/post-comments/style.css 359 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 396 B
build/block-library/blocks/post-featured-image/editor.css 397 B
build/block-library/blocks/post-featured-image/style-rtl.css 156 B
build/block-library/blocks/post-featured-image/style.css 156 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 391 B
build/block-library/blocks/post-template/style.css 392 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 60 B
build/block-library/blocks/post-title/style.css 60 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 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 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 262 B
build/block-library/blocks/query-pagination/editor.css 255 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query-title/editor-rtl.css 85 B
build/block-library/blocks/query-title/editor.css 85 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 220 B
build/block-library/blocks/quote/theme.css 222 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 374 B
build/block-library/blocks/search/style.css 375 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 250 B
build/block-library/blocks/separator/style.css 250 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 769 B
build/block-library/blocks/site-logo/editor.css 769 B
build/block-library/blocks/site-logo/style-rtl.css 165 B
build/block-library/blocks/site-logo/style.css 165 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 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 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 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 636 B
build/block-library/blocks/template-part/editor.css 635 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/term-description/editor-rtl.css 90 B
build/block-library/blocks/term-description/editor.css 90 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 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 815 B
build/block-library/common.css 812 B
build/block-library/index.min.js 148 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/theme-rtl.css 665 B
build/block-library/theme.css 669 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 45.7 kB
build/compose/index.min.js 10.4 kB
build/core-data/index.min.js 12.4 kB
build/customize-widgets/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 614 B
build/data/index.min.js 7.1 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 428 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.46 kB
build/edit-navigation/index.min.js 15.3 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29.3 kB
build/edit-post/style-rtl.css 7.22 kB
build/edit-post/style.css 7.22 kB
build/edit-site/style-rtl.css 5.54 kB
build/edit-site/style.css 5.54 kB
build/edit-widgets/index.min.js 15.7 kB
build/edit-widgets/style-rtl.css 4.12 kB
build/edit-widgets/style.css 4.13 kB
build/editor/index.min.js 37.5 kB
build/editor/style-rtl.css 3.78 kB
build/editor/style.css 3.77 kB
build/element/index.min.js 3.17 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.93 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.6 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.72 kB
build/keycodes/index.min.js 1.3 kB
build/list-reusable-blocks/index.min.js 1.85 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 845 B
build/nux/index.min.js 2.03 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.83 kB
build/primitives/index.min.js 921 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.63 kB
build/reusable-blocks/index.min.js 2.19 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.52 kB
build/shortcode/index.min.js 1.48 kB
build/token-list/index.min.js 562 B
build/url/index.min.js 1.74 kB
build/viewport/index.min.js 1.02 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.11 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@Mamaduka
Copy link
Member

Mamaduka commented Oct 6, 2021

Hi, Nik

I just wanted to note that we want this component to be backward compatible with unitless values. See #33679.

We have unit tests in place, so the regression should be easy to catch.

@jasmussen
Copy link
Contributor

Thanks for starting work on this one. It's a big one, it's a difficult one, and it's so nice to get something on the blank canvas. Here's a GIF of what I saw in a quick testing:

font

I'll need to come back to this one with deeper thoughts, but a few quick superficial observations:

This is just one small ingredient of a large effort, nice work, and I think many of the pieces will start falling into place once we get this one down.

@ntsekouras
Copy link
Contributor Author

Thanks for the first tests @jasmussen and great remarks!

Would it make sense to make progress on the generic input-field component work?
The goal of the toolspanel is, in part, to retire the many Reset buttons strewn across the inspector. I wonder if we should tie this PR to Typography: Switch to ToolsPanel for display UI #33744 by @aaronrobertshaw?

Yes, it makes sense to fix/augment existing components that are going to be used. I made the draft to pause a bit and check some related PRs like the ToolsPanel that handle the reset, etc..

The "Custom" option in the dropdown didn't seem to work for me, but hitting the settings icon did.

I know about that and haven't decided yet how to handle the options (default + custom) as it will also be coupled with ToggleGroupControl.

@aaronrobertshaw
Copy link
Contributor

The goal of the toolspanel is, in part, to retire the many Reset buttons strewn across the inspector. I wonder if we should tie this PR to Typography: Switch to ToolsPanel for display UI #33744 by @aaronrobertshaw?

Thanks for the ping. It's great to see progress on the new typography components. 👍

I did have some tweaks to the current FontSizePicker included in #33744 to allow the omission of the reset button and fix up spacing when used within the ToolsPanel. Those tweaks have just been removed from that PR although I planned to create a separate PR with them tomorrow.

If you'd prefer for me to put those changes on ice until this PR evolves, let me know. Even as it is now, the version of FontSizePicker in this PR would fit a little nicer within the ToolsPanel than the version on trunk.

Other than the reset button, the only other tweak for the ToolsPanel would be removing the bottom margin so that the panel's grid can handle the spacing.

Applying these changes alongside #33744 appears to work well and I'd expect it to evolve without many conflicts.

Screen.Recording.2021-10-07.at.5.23.22.pm.mp4

I did encounter one small issue while testing which was in a couple of themes the select options included NaN in them. I believe this was due to the theme's font size presets using CSS vars for the size property. The Q theme is one example.

Screen Shot 2021-10-07 at 5 20 10 pm

@ciampo
Copy link
Contributor

ciampo commented Oct 7, 2021

Would it make sense to make progress on the generic input-field component work? The 40px input fields from The Global Styles Interface #34574 and Sidebar Controls & Component System #27331 might benefit the work here, just to have in place.

Hey @jasmussen , that definitely makes sense (although in a separate issue/PR). We should be adding any changes that are necessary to improve the typography panel in tracking issue #34345, so that we can take it from there and coordinate around these tasks.

With respect to what @aaronrobertshaw said, that sounds like a good plan to me!

@ntsekouras
Copy link
Contributor Author

I did encounter one small issue while testing which was in a couple of themes the select options included NaN in them. I believe this was due to the theme's font size presets using CSS vars for the size property.

I'll look into this, thanks!

Regarding the reset I'm going to try to maybe have a first iteration with the reset button and the current custom size input with the units (no slider). We'll see how this goes and has to do with the other tools panel PRs. Remove the reset button will be easy 😄 .

@ntsekouras ntsekouras force-pushed the try/font-size-control-changes branch 2 times, most recently from 04ce574 to 57c323e Compare October 8, 2021 05:40
@ntsekouras
Copy link
Contributor Author

I've updated the control to switch to ToggleGroupControl conditionally and added a simple story for start to test there as well(Different Control By Size under FontSizePicker).

I think it's ready for another round of testing with different themes etc 😄

list-style-type: none;
padding: $grid-unit-10;
cursor: default;
line-height: $icon-size + $grid-unit-05;


&.has-hint {
grid-template-columns: auto auto 30px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we handle this better? 🤔 --cc @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered using the Grid component instead of using CSS styles?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand on what I should look for here? Happy to help if I can.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean was to refactor this component to use internally the Grid component — instead of using custom CSS rules for the grid layout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make the minimum changes in CustomSelectControl and I'm not sure there are some better rules I should use - or even maybe flex? Maybe I'll consider splitting these changes to a separate PR as Marco suggested..

@jasmussen
Copy link
Contributor

jasmussen commented Oct 8, 2021

Took this one for a quick spin, and my goodness, even though parts are missing, this is just a big step forward:
typography

In the mockups, the fields use a new all-caps subheading style. I imagine this is something we can follow up with separately, but for now we should at least remove the thin font weight from this one:
Screenshot 2021-10-08 at 14 44 00

Instead of font-weigh: 200; it should just be normal, and you can then set it to color: $gray-700; and perhaps give it $grid-unit-05 left margin.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Thank you for starting work on this PR, @ntsekouras ! As others have also said, it definitely represents a big improvement over the current UI.

I've been taking a more deeper look at the code and left a few comments. We should also rebase this PR to include a few changes to the FontSizePicker component that got merged in the meantime.

Regarding the proposal of rewriting these components to TypeScript and use Emotion, I'm more than happy to help (either directly, or indirectly with advice and reviews).

Finally, I just wanted to cross-post the fact that #35451 may contain some overlapping changes (cc'ing the PR author, @aaronrobertshaw )

);

const baseClassName = 'components-font-size-picker';
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this classname existed before this PR, but it got me thinking — it'd be also great if we could ride the wave of changes to FontSizePicker to also switch its styling from Sass to Emotion, and convert it to TypeScipt (of course in a separate PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, but let's do this separately..

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that sounds like a good follow-up

packages/components/src/font-size-picker/index.js Outdated Show resolved Hide resolved
Comment on lines +166 to +170
{ item.hint && (
<span className="components-custom-select-control__item-hint">
{ item.hint }
</span>
) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to split changes to CustomSelectControl in a separate PR, where the relevant documentation is also updated and a CHANGELOG entry is added.

The current PR will keep focusing on the FontSizePicker, making changes more contained and easier to find in the future.


Separately from this, I also wonder if we should rewrite the CustomSelectControl component to use Emotion for styling and rewrite it to TypeScript. These improvements would ease the integration of these components with the Typography panel and give more confidence to our refactors (via TYpeScript's static linting)

@ntsekouras
Copy link
Contributor Author

In the mockups, the fields use a new all-caps subheading style. I imagine this is something we can follow up with separately,

Yes. I saw that but this affects all control labels and we should be consistent. In a separate PR we should update all controls to uppercase - css probably(?) as third party controls can be there..

we should at least remove the thin font weight from this one:

Good call 😄 - I updated the styles.

@ciampo
Copy link
Contributor

ciampo commented Oct 12, 2021

Yes. I saw that but this affects all control labels and we should be consistent. In a separate PR we should update all controls to uppercase - css probably(?) as third party controls can be there..

I believe a similar issue was raised in #35400 (comment). We've got #35464 open to potentially update the Heading component styles to match the latest design requirements.

@ntsekouras ntsekouras marked this pull request as ready for review October 14, 2021 10:01
@jasmussen
Copy link
Contributor

jasmussen commented Oct 14, 2021

Taking this one for a spin again, it feels pretty solid to me:
font size

There are a number of small things we need to fix, but I don't think they need to happen as part of this PR. For example, the custom font size icon is offset from the right:
Screenshot 2021-10-14 at 14 05 56

This padding is also what makes the toggle wider, and scales down the icon inside:

Screenshot 2021-10-14 at 14 06 48

The right way to fix that is to revisit the CSS that adds this padding in the first place, which appears to be this:

.components-button.is-small.has-icon:not(.has-text) {
    padding: 0 8px;
    width: 24px;
}

However in the name of shipping this, we could add the following in the mean time? Happy to push if need be. This would fix it:

.components-font-size-picker__header .components-button.is-small.has-icon:not(.has-text) {
    min-width: $icon-size;
    padding: 0;
}

Screenshot 2021-10-14 at 14 10 23

Screenshot 2021-10-14 at 14 12 45

@jasmussen
Copy link
Contributor

Also wanted to note that the items inside the segmented control follow the order of font sizes defined in theme.json. I had them in large to small descending order, and when correcting them to be small to large, this is reflected in the component:

			"fontSizes": [
				{
					"slug": "small",
					"size": "12px",
					"name": "Small"
				},
				{
					"slug": "large",
					"size": "18px",
					"name": "Large"
				},
				{
					"slug": "medium",
					"size": "14px",
					"name": "Medium"
				},
				{
					"slug": "xl",
					"size": "48px",
					"name": "XL"
				},
				{
					"slug": "xxl",
					"size": "64px",
					"name": "XXL"
				}
			],

Screenshot 2021-10-14 at 14 15 19

👍 👍

@jasmussen
Copy link
Contributor

jasmussen commented Oct 14, 2021

I can also confirm that if I add additional values, more than 5, they show up in the custom dropdown:
Screenshot 2021-10-14 at 14 17 03

Edit: Actually, let's replace that font-weight: 200; with a $gray-700 color.

And the dropdown looks reasonably good, and can be improved separately. From a design point of view, I'd be happy to greenlight this one if we can fix the custom font size icon, as outlined here, and I'd also personaly be okay with bespoke CSS to fix it, and then follow up on that separately.

@jasmussen
Copy link
Contributor

Since there's already a CSS file that customizes this panel, I went ahead and pushed a small fix to the icon size, padding, position and width, and changed the font weight in the dropdown. Looks good to me now:
Screenshot 2021-10-14 at 14 23 30
Screenshot 2021-10-14 at 14 23 39

@jasmussen
Copy link
Contributor

Tested other instances of the CustomSelectControl, to verify they hadn't regressed. It's used for font appearance, and for date format. Easy ways to test is to insert a Post Date block and check the format in the inspector:
post comment date

Or the tagline block and check the font appearance:
tagline

All looks solid to me.

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.

This feels solid to me, and a big step forward. From the design side, there are a few small things to follow up on, but most of them fall in the category of general components work (40px input fields, consume components without having to write bespoke CSS), and not something that should block this PR.

I tested it with a few font size presets, and with many. In both cases the experience works well for me, using both mouse and keyboard. I also verified that the enhancements made to the CustomSelectControl have not caused trouble with any of the existing usages of the component. In that light, this one feels safe.

I'd be happy to land this one as a first iteration and then follow up if anything appears.

@geriux
Copy link
Member

geriux commented Oct 14, 2021

Hey @ntsekouras 👋 I just tested these changes on mobile and it's working well, thanks for the ping! 👍

@ntsekouras ntsekouras merged commit 21831a9 into trunk Oct 14, 2021
@ntsekouras ntsekouras deleted the try/font-size-control-changes branch October 14, 2021 13:09
@ntsekouras
Copy link
Contributor Author

Thank you all for your help here!!! 🙏

@github-actions github-actions bot added this to the Gutenberg 11.8 milestone Oct 14, 2021
@jasmussen
Copy link
Contributor

🔥

@ciampo
Copy link
Contributor

ciampo commented Oct 14, 2021

It's good to see progress happening so fast!

One thing that I'd like to note, though, is that we included changes to CustomSelectControl which I believe should have been split to a separate PR (could we do it in a follow-up PR at least?) and that we should try to avoid adding more hardcoded classnames to component's styles.

Introducing a lot of fast changes, especially in a package like the components package, can become counter-productive very quickly.

@ciampo
Copy link
Contributor

ciampo commented Oct 15, 2021

Updated CustomSelectControl docs in this follow-up PR #35673

@ntsekouras
Copy link
Contributor Author

One thing that I'd like to note, though, is that we included changes to CustomSelectControl which I believe should have been split to a separate PR
Introducing a lot of fast changes, especially in a package like the components package, can become counter-productive very quickly.

You're right about that and I was thinking maybe it's better to make this prop experimental for now and revisit when the typescript/css-in-js is handled for this component ? 🤔

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 [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants