Size Change: -56 B (0%)

Total Size: 1.08 MB

_{compressed-size-action}

Thanks! :)

Is it possible to fix the issue in FontSizePicker instead?

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

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.

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.

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."

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

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.

What was the behavior in WP 5.7?

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.

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.

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?

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

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.

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...

It looks like we already have hasUnits check for values:

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.

@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.

@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.

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

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

Whatever works best for you :)

@aristath, @youknowriad

Added unit tests.

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

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