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

FontSizePicker: Use number values when the initial value is a number #33679

Merged
merged 5 commits into from
Jul 30, 2021

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Jul 26, 2021

Description

Updates onChange logic in FontSizePicker component. When the component detects that value or first object in fonSizes doesn't use units, it will pass the number value to the onChange callback.

Fixes #33651.
Fixes #33653.

How has this been tested?

Tested with plugin provided in original issue report - https://github.com/NasKadir123/textBlock

Types of changes

Bugfix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@Mamaduka Mamaduka requested a review from ellatrix as a code owner July 26, 2021 06:02
@Mamaduka Mamaduka self-assigned this Jul 26, 2021
@Mamaduka Mamaduka added [Package] Block editor /packages/block-editor [Type] Regression Related to a regression in the latest release labels Jul 26, 2021
@Mamaduka Mamaduka requested a review from aristath July 26, 2021 06:03
@Mamaduka Mamaduka added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jul 26, 2021
@github-actions
Copy link

github-actions bot commented Jul 26, 2021

Size Change: -56 B (0%)

Total Size: 1.08 MB

Filename Size Change
build/api-fetch/index.min.js 2.44 kB +1 B (0%)
build/block-editor/index.min.js 127 kB +23 B (0%)
build/block-editor/style-rtl.css 14 kB -3 B (0%)
build/block-editor/style.css 14 kB -3 B (0%)
build/block-library/index.min.js 147 kB +1 B (0%)
build/components/index.min.js 197 kB +14 B (0%)
build/core-data/index.min.js 12.6 kB +114 B (+1%)
build/edit-navigation/index.min.js 13.9 kB +3 B (0%)
build/edit-post/index.min.js 59.4 kB +2 B (0%)
build/editor/index.min.js 38.3 kB -207 B (-1%)
build/format-library/index.min.js 5.72 kB -1 B (0%)
build/server-side-render/index.min.js 1.64 kB +1 B (0%)
build/widgets/index.min.js 6.48 kB -1 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 1.12 kB
build/admin-manifest/index.min.js 1.46 kB
build/annotations/index.min.js 2.93 kB
build/autop/index.min.js 2.28 kB
build/blob/index.min.js 673 B
build/block-directory/index.min.js 6.62 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 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 112 B
build/block-library/blocks/audio/style.css 112 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 475 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 603 B
build/block-library/blocks/button/style.css 602 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 375 B
build/block-library/blocks/buttons/style.css 375 B
build/block-library/blocks/calendar/style-rtl.css 208 B
build/block-library/blocks/calendar/style.css 208 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 190 B
build/block-library/blocks/columns/editor.css 190 B
build/block-library/blocks/columns/style-rtl.css 475 B
build/block-library/blocks/columns/style.css 476 B
build/block-library/blocks/cover/editor-rtl.css 670 B
build/block-library/blocks/cover/editor.css 670 B
build/block-library/blocks/cover/style-rtl.css 1.22 kB
build/block-library/blocks/cover/style.css 1.23 kB
build/block-library/blocks/embed/editor-rtl.css 486 B
build/block-library/blocks/embed/editor.css 486 B
build/block-library/blocks/embed/style-rtl.css 401 B
build/block-library/blocks/embed/style.css 400 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 301 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 711 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 704 B
build/block-library/blocks/gallery/editor.css 705 B
build/block-library/blocks/gallery/style-rtl.css 1.06 kB
build/block-library/blocks/gallery/style.css 1.06 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 160 B
build/block-library/blocks/group/editor.css 160 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 93 B
build/block-library/blocks/group/theme.css 93 B
build/block-library/blocks/heading/editor-rtl.css 152 B
build/block-library/blocks/heading/editor.css 152 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 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 281 B
build/block-library/blocks/html/editor.css 281 B
build/block-library/blocks/image/editor-rtl.css 729 B
build/block-library/blocks/image/editor.css 727 B
build/block-library/blocks/image/style-rtl.css 481 B
build/block-library/blocks/image/style.css 485 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 286 B
build/block-library/blocks/latest-comments/style.css 286 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 526 B
build/block-library/blocks/latest-posts/style.css 524 B
build/block-library/blocks/list/style-rtl.css 63 B
build/block-library/blocks/list/style.css 63 B
build/block-library/blocks/media-text/editor-rtl.css 263 B
build/block-library/blocks/media-text/editor.css 264 B
build/block-library/blocks/media-text/style-rtl.css 492 B
build/block-library/blocks/media-text/style.css 489 B
build/block-library/blocks/more/editor-rtl.css 434 B
build/block-library/blocks/more/editor.css 434 B
build/block-library/blocks/navigation-link/editor-rtl.css 474 B
build/block-library/blocks/navigation-link/editor.css 473 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/editor-rtl.css 1.69 kB
build/block-library/blocks/navigation/editor.css 1.69 kB
build/block-library/blocks/navigation/style-rtl.css 1.65 kB
build/block-library/blocks/navigation/style.css 1.66 kB
build/block-library/blocks/navigation/view.min.js 2.84 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 310 B
build/block-library/blocks/page-list/editor.css 311 B
build/block-library/blocks/page-list/style-rtl.css 240 B
build/block-library/blocks/page-list/style.css 240 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 247 B
build/block-library/blocks/paragraph/style.css 248 B
build/block-library/blocks/post-author/editor-rtl.css 209 B
build/block-library/blocks/post-author/editor.css 209 B
build/block-library/blocks/post-author/style-rtl.css 183 B
build/block-library/blocks/post-author/style.css 184 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-content/editor-rtl.css 139 B
build/block-library/blocks/post-content/editor.css 139 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 338 B
build/block-library/blocks/post-featured-image/editor.css 338 B
build/block-library/blocks/post-featured-image/style-rtl.css 141 B
build/block-library/blocks/post-featured-image/style.css 141 B
build/block-library/blocks/post-template/editor-rtl.css 100 B
build/block-library/blocks/post-template/editor.css 99 B
build/block-library/blocks/post-template/style-rtl.css 379 B
build/block-library/blocks/post-template/style.css 380 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 362 B
build/block-library/blocks/pullquote/style.css 361 B
build/block-library/blocks/pullquote/theme-rtl.css 169 B
build/block-library/blocks/pullquote/theme.css 169 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 270 B
build/block-library/blocks/query-pagination/editor.css 262 B
build/block-library/blocks/query-pagination/style-rtl.css 168 B
build/block-library/blocks/query-pagination/style.css 168 B
build/block-library/blocks/query-title/editor-rtl.css 86 B
build/block-library/blocks/query-title/editor.css 86 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 169 B
build/block-library/blocks/quote/style.css 169 B
build/block-library/blocks/quote/theme-rtl.css 221 B
build/block-library/blocks/quote/theme.css 221 B
build/block-library/blocks/rss/editor-rtl.css 201 B
build/block-library/blocks/rss/editor.css 202 B
build/block-library/blocks/rss/style-rtl.css 290 B
build/block-library/blocks/rss/style.css 290 B
build/block-library/blocks/search/editor-rtl.css 211 B
build/block-library/blocks/search/editor.css 211 B
build/block-library/blocks/search/style-rtl.css 372 B
build/block-library/blocks/search/style.css 373 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 251 B
build/block-library/blocks/separator/style.css 251 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 476 B
build/block-library/blocks/shortcode/editor.css 476 B
build/block-library/blocks/site-logo/editor-rtl.css 465 B
build/block-library/blocks/site-logo/editor.css 465 B
build/block-library/blocks/site-logo/style-rtl.css 154 B
build/block-library/blocks/site-logo/style.css 154 B
build/block-library/blocks/site-tagline/editor-rtl.css 87 B
build/block-library/blocks/site-tagline/editor.css 87 B
build/block-library/blocks/site-title/editor-rtl.css 85 B
build/block-library/blocks/site-title/editor.css 85 B
build/block-library/blocks/social-link/editor-rtl.css 164 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 800 B
build/block-library/blocks/social-links/editor.css 799 B
build/block-library/blocks/social-links/style-rtl.css 1.34 kB
build/block-library/blocks/social-links/style.css 1.34 kB
build/block-library/blocks/spacer/editor-rtl.css 308 B
build/block-library/blocks/spacer/editor.css 308 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 478 B
build/block-library/blocks/table/editor.css 478 B
build/block-library/blocks/table/style-rtl.css 480 B
build/block-library/blocks/table/style.css 480 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 551 B
build/block-library/blocks/template-part/editor.css 550 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 569 B
build/block-library/blocks/video/editor.css 570 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 1.29 kB
build/block-library/common.css 1.29 kB
build/block-library/editor-rtl.css 9.83 kB
build/block-library/editor.css 9.83 kB
build/block-library/reset-rtl.css 514 B
build/block-library/reset.css 515 B
build/block-library/style-rtl.css 10.3 kB
build/block-library/style.css 10.3 kB
build/block-library/theme-rtl.css 692 B
build/block-library/theme.css 693 B
build/block-serialization-default-parser/index.min.js 1.3 kB
build/block-serialization-spec-parser/index.min.js 3.06 kB
build/blocks/index.min.js 47.2 kB
build/components/style-rtl.css 15.9 kB
build/components/style.css 15.9 kB
build/compose/index.min.js 10.2 kB
build/customize-widgets/index.min.js 10.3 kB
build/customize-widgets/style-rtl.css 1.48 kB
build/customize-widgets/style.css 1.48 kB
build/data-controls/index.min.js 831 B
build/data/index.min.js 7.22 kB
build/date/index.min.js 31.8 kB
build/deprecated/index.min.js 738 B
build/dom-ready/index.min.js 576 B
build/dom/index.min.js 4.78 kB
build/edit-navigation/style-rtl.css 3.12 kB
build/edit-navigation/style.css 3.12 kB
build/edit-post/classic-rtl.css 483 B
build/edit-post/classic.css 483 B
build/edit-post/style-rtl.css 7.25 kB
build/edit-post/style.css 7.24 kB
build/edit-site/index.min.js 26 kB
build/edit-site/style-rtl.css 5.04 kB
build/edit-site/style.css 5.03 kB
build/edit-widgets/index.min.js 16.2 kB
build/edit-widgets/style-rtl.css 3.99 kB
build/edit-widgets/style.css 3.99 kB
build/editor/style-rtl.css 3.96 kB
build/editor/style.css 3.96 kB
build/element/index.min.js 3.44 kB
build/escape-html/index.min.js 739 B
build/format-library/style-rtl.css 668 B
build/format-library/style.css 669 B
build/hooks/index.min.js 1.76 kB
build/html-entities/index.min.js 628 B
build/i18n/index.min.js 3.73 kB
build/is-shallow-equal/index.min.js 710 B
build/keyboard-shortcuts/index.min.js 1.74 kB
build/keycodes/index.min.js 1.49 kB
build/list-reusable-blocks/index.min.js 2.07 kB
build/list-reusable-blocks/style-rtl.css 842 B
build/list-reusable-blocks/style.css 842 B
build/media-utils/index.min.js 3.08 kB
build/notices/index.min.js 1.07 kB
build/nux/index.min.js 2.31 kB
build/nux/style-rtl.css 745 B
build/nux/style.css 742 B
build/plugins/index.min.js 1.99 kB
build/primitives/index.min.js 1.06 kB
build/priority-queue/index.min.js 791 B
build/react-i18n/index.min.js 924 B
build/redux-routine/index.min.js 2.82 kB
build/reusable-blocks/index.min.js 2.56 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.8 kB
build/shortcode/index.min.js 1.68 kB
build/token-list/index.min.js 848 B
build/url/index.min.js 1.95 kB
build/viewport/index.min.js 1.28 kB
build/warning/index.min.js 1.16 kB
build/widgets/style-rtl.css 1.04 kB
build/widgets/style.css 1.05 kB
build/wordcount/index.min.js 1.24 kB

compressed-size-action

@olafleur
Copy link

Thanks! :)

@youknowriad
Copy link
Contributor

Is it possible to fix the issue in FontSizePicker instead?

@youknowriad
Copy link
Contributor

Asking because this will fix withFontSizes but FontSizePicker direct usage still remains broken right?

@Mamaduka
Copy link
Member Author

Asking because this will fix withFontSizes but FontSizePicker direct usage still remains broken right?

The FontSizePicker now will return units and blocks can use those directly in attributes - #31314.

If we have to parse units into numbers there as well, then using UnitControl doesn't really make sense.

@youknowriad
Copy link
Contributor

I was thinking something like: if the passed (previous) value passed to FontSizePicker is a number and the selected unit is "px", call onChange with just the number (and not the full string).

That said, I still prefer this PR because the proposal is a hack but the hack might be necessary depending on the scale of the breaking change.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Maybe this is good enough as both reported issues are specifically about withFontSizes

@Mamaduka
Copy link
Member Author

So far, I've only seen reports about withFontSizes. It might be better to add a proposed hack if we see reports for the FontSizePicker component.

Side note: The problem can also be fixed by changing attribute type from "number" to "string."

@youknowriad
Copy link
Contributor

One question though about the fix here. It seems that the proposed fix assumes that only number attributes are supported with withFontSize. Won't that break if we use that with the default FontSizePicker. Meaning someone could select "2em" but it would get applied as "2px" ?

@Mamaduka
Copy link
Member Author

The withFontSize was only using numbers before, so users/devs expect numbers in their attributes.

But yeah, sadly, unit control values will have no effect.

@youknowriad
Copy link
Contributor

What was the behavior in WP 5.7?

@Mamaduka
Copy link
Member Author

Mamaduka commented Jul 28, 2021

The FontSizePicker was using NumberControl, so onChange was returning numbers.

What do you think about this idea?

  • Use parseCSSUnitValue to parse values.
  • If the unit is px, return parsed number, otherwise return the whole unit.

@Mamaduka
Copy link
Member Author

Currently, I can think of the one issue with my proposed solution: if the user changes unit and attribute type is a number, it will trigger block validation.

@youknowriad
Copy link
Contributor

The FontSizePicker was using NumberControl, so onChange was returning numbers.

So we introduced units only in 5.7? In my mind, we did introduce that before but maybe I'm wrong?

@Mamaduka
Copy link
Member Author

I think we introduced the units before, but we update NumberControl to UnitControl in FontSizePicker in 5.8. This is the PR - #31314.

@youknowriad
Copy link
Contributor

Ok, so it seems the best approach would be to revert the FontSizePicker to pixel only (to avoid any breaking changes) and have a way to trigger multi-units by padding a units array or something like that (or potentially a boolean) which we should enable for core blocks all hooks. Is something like that possible?

Also curious about @aristath's opinion here.

@aristath
Copy link
Member

Making the FontSizePicker use UnitControl instead of NumberControl was a significant improvement... Reverting that would be a big regression for designers.
What if, instead of reverting an improvement, we introduced a package/method to automatically convert all units to pixel values when needed? It should be possible to convert 2em to 32px and get 32 as a number...

@Mamaduka
Copy link
Member Author

It looks like we already have hasUnits check for values:

const hasUnits =
isString( value ) ||
( fontSizes[ 0 ] && isString( fontSizes[ 0 ].size ) );

I think we can use that check and do following:

onChange( hasUnits ? nextSize : parseInt( nextSize, 10 ) );

This way, we'll consider the block's current attribute value and pre-defined sizes, allowing us to use units or numbers.

I will update PR shortly to test this case.

@youknowriad
Copy link
Contributor

@aristath I'm not suggesting to revert but to make it optional (and enabled for all Core things) because switching the value from number to attribute is breaking all plugins that rely on the "value" of FontSizePicker being a number.

@Mamaduka Mamaduka force-pushed the fix/with-font-sizes-parse-units branch from dcd67bd to ccd9547 Compare July 29, 2021 11:06
@Mamaduka Mamaduka requested a review from ajitbohra as a code owner July 29, 2021 11:06
@Mamaduka
Copy link
Member Author

@youknowriad, @aristath

Removed original fix and updated onChange logic in FontSizePicker component. When the component detects that value or first object in fonSizes doesn't use units, it will pass the number to the onChange callback.

It should fix the BC issue and allow developers to use units with FontSizePicker.

@youknowriad
Copy link
Contributor

@Mamaduka maybe I'm asking too much but I feel like we should have an e2e test with the provided plugin to avoid future regressions. WDYT?

@Mamaduka
Copy link
Member Author

I think this case is more suited for unit tests - it should return number if value|sizes are numbers.

Or do you want to test that it doesn't trigger block validation and attributes are correctly defined?.

@youknowriad
Copy link
Contributor

Whatever works best for you :)

@Mamaduka
Copy link
Member Author

@aristath, @youknowriad

Added unit tests.

@Mamaduka Mamaduka changed the title withFontSizes: Parse units into a numbers FontSizePicker: Use number values when the initial value is a number Jul 30, 2021
@Mamaduka Mamaduka added [Package] Components /packages/components and removed [Package] Block editor /packages/block-editor labels Jul 30, 2021

describe( 'FontSizePicker', () => {
describe( 'onChange values', () => {
it( 'should not use units when the initial value is a number', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that the units input is not render or do you think it should be rendered in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean units "switcher"? It's is still rendered but has no effect since we only use number values.

I think toggling between NumberControl and UnitControl based on has units value will introduce more complexity to the component.

CleanShot 2021-07-30 at 13 37 06

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 toggling between NumberControl and UnitControl based on has units value will introduce more complexity to the component.

Can't we just pass an empty array of units or something like that in that case? It feels weird for a user to select "em" but have pixels applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. It will now only display the "PX" unit label when there are no units.

@youknowriad
Copy link
Contributor

This is great for me, thanks for all the iterations.

@Mamaduka
Copy link
Member Author

Not a problem. I'm just happy we were able to find the right way to fix this issue.

@Mamaduka Mamaduka merged commit 1ba0bff into trunk Jul 30, 2021
@Mamaduka Mamaduka deleted the fix/with-font-sizes-parse-units branch July 30, 2021 11:21
@github-actions github-actions bot added this to the Gutenberg 11.3 milestone Jul 30, 2021
desrosj pushed a commit that referenced this pull request Aug 30, 2021
…33679)

* FontSizePicker: Don't use units if the value is a number
* Add unit tests
* Disable units when we have number values
@desrosj desrosj mentioned this pull request Aug 30, 2021
7 tasks
desrosj added a commit that referenced this pull request Sep 1, 2021
* Fix API docs generation (#33384)

* Docs: use markdown headings instead of links for API declarations (#33381)

* Docs: Run Prettier after updating API in documentation (#33498)

(cherry picked from commit 626f233)

* Use tabs instead of spaces in block transform doc example (#33549)

(cherry picked from commit 8afca1e)

* Fix metabox reordering (#30617).

* Block editor: don't render layout and duotone styles in between blocks (#32083)

* Widgets: Allow HTML tags in description (#33814)

* Widgets: Allow HTML tags in description

* Use `dangerouslySetInnerHTML`

Avoid `<div />` inside the `<p />` tag

* Describe by dangerouslySetInnerHTML is used

* Use safeHTML

* Update comment

* Editor: Set 'hide_empty' for the most used terms query (#33457)

Don't include terms that aren't assigned to any posts as "most used" terms.

* Update widget editor help links to point to the new support article (#33482)

* If select-all fires in .editor-post-title__input, end the process.. (#33621)

* Writing flow: select all: remove early return for post title (#33699)

* Call onChangeSectionExpanded conditionally (#33618)

* FontSizePicker: Use number values when the initial value is a number (#33679)

* FontSizePicker: Don't use units if the value is a number
* Add unit tests
* Disable units when we have number values

* Fix justification for button block when selected (#33739)

* Remove margin setting, auto right conflict with justify buttons

* Per review, add little margin back

* Add error boundaries to widget screens (#33771)

* Add error boundary to edit widgets screen

* Add error boundary to customize widgets

* Refactor sidebar controls provider to application level so that its state is not lost when re-initializing

* Revert "Refactor sidebar controls provider to application level so that its state is not lost when re-initializing"

This reverts commit 7d607ff.

* Remove rebootability from customize widgets

* Remove debug code

* Fix insertion point in Widgets editors (#33802)

* Default batch processor: Respect the batch endpoint's maxItems (#34280)

This updates the default batch processor to make multiple batch requests
if the number of requests to process exceeds the number of requests that
the batch endpoint can handle.

We determine the number of requests that the batch endpoint can handle
by making a preflight OPTIONS request to /batch/v1. By default it is 25
requests.

See https://make.wordpress.org/core/2020/11/20/rest-api-batch-framework-in-wordpress-5-6/.

* Fix button block focus trap after a URL has been added (#34314)

* Rework button block link UI to match RichText format implementation

* Refine some more, determine visibility by selection and url state

* Add e2e test

* Also focus rich text when unlinking using a keyboard shortcut

* Text for dropdown fields within legacy widgets in the Customizer is off centered (#34076)

* Fix ESLint errors reported

* Regenerate autogenerated docs

* Update the `INSERTER_SEARCH_SELECTOR` path.

This is a partial cherry pick of 2356b2d in order to fix the performance tests.

Co-authored-by: André <nosolosw@users.noreply.github.com>
Co-authored-by: JuanMa <juanma.garrido@gmail.com>
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
Co-authored-by: Jeff Bowen <jblz@users.noreply.github.com>
Co-authored-by: Bruno Ribarić <43731400+ribaricplusplus@users.noreply.github.com>
Co-authored-by: Ella van Durpe <4710635+ellatrix@users.noreply.github.com>
Co-authored-by: Petter Walbø Johnsgård <petter@dekode.no>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Co-authored-by: Hiroshi Urabe <mail@torounit.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Marcus Kazmierczak <marcus@mkaz.com>
Co-authored-by: Robert Anderson <robert@noisysocks.com>
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

withFontSizes bug in WordPress 5.8 release FontSizePicker is not working with value type "number"
4 participants