Fix default unit issue for tag cloud block

[akasunil]

Fixed #58885

What?

Tag cloud block default size is set in PT unit, but when PT unit is not available, it vanish on unit change.

Why?

Converting default value into first available unit.

also modified parseQuantityAndUnitFromRawValue() to return default unit value when match not found. Before it was returning undefined even when match isn't found.

Testing Instructions

• Add a Tag cloud block.
• In the settings panel, in the options "smallest size" and "largest size", change the unit.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: akasunil <sunil25393@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[akasunil]

@Mamaduka can you please review this PR ?

[carolinan]

@sunil25393 Hi. Did you explore if it was possible to make the pt unit available?

[akasunil]

@carolinan Here, the PT unit is utilized as the default font size value, and available units are obtained from the spacing.units setting, which can be changed by the theme.

As you can see, the twentyTwentyFour theme's spacing units do not include the PT unit. So, even if I add PT units to this list, the problem will persist.

To overcome this problem, I have used getValidParsedQuantityAndUnit to determine whether the unit is available or not. If the unit is not made available by the theme. Then we may select the first unit from the list as the default unit.

Please let me know if you have any alternative approaches.

[carolinan]

Spacing units are separate from font size units. If spacing units affects typography then that is a separate bug that needs to be reported and solved separately from this issue and pr.

[akasunil]

If we remove the use of spacing.units ( as you mentioned spacing units should not be used for font sizes ), then i guess we can include PT unit in default units.

Also, we don't have global styles setting for font size units like spacing.units, so we just have to use an array of hard coded units like here.

i'll update my PR accordingly.

[akasunil]

Added same units as we use for font size picker including PT unit. And removed use of spacing units setting for font sizes.

[ntsekouras]

It feels to me that we should always honor the spacing units and we shouldn't remove this. Having said that, since the tag cloud has this nuance with the previous default value, maybe we should just append the pt to the availableUnits in either case (spacing.units or defaults). What do you think?

[carolinan]

It is not logical to me that spacing units are used as the font size unit and I would like to understand why they were used this way?

I don't think pt is used for spacing.

[ntsekouras]

That's a good question! Do we have a different theme.json setting for font size units? I searched a bit and I think not.. We should probably use the same values we use for setting font sizes in typography panel (size).

My point is, if we have such a setting and we were using the wrong one, we should honor it. I'll cc @MaggieCabrera if she can provide some feedback.

[carolinan]

I vaguely remember having a discussion about font size unit block support with @t-hamano but I can't find that issue or PR now.

BTW unit support is broken in block.json #47519

[t-hamano]

It is not logical to me that spacing units are used as the font size unit

I completely agree with this point, but given that theme.json doesn't support font size units, I think we need to respect spacing.units for now to maintain backward compatibility.

How about keeping the current specs but adding the pt unit?

// The `pt` unit is used as the default value and is therefore
// always considered an available unit.
const units = useCustomUnits( {
availableUnits: availableUnits
	? [ ...availableUnits, 'pt' ]
	: [ '%', 'px', 'em', 'rem', 'pt' ],
} );

[akasunil]

Thank you @t-hamano, Let me know If we all are agree with the above proposed solution, ill update the PR

Also, Shouldn't we consider having a unit setting for font size in theme.json?

[t-hamano]

I believe this approach is the best one to maintain backward compatibility and users to reselect the pt unit.
allow

Shouldn't we consider having a unit setting for font size in "theme.json"?

Of course, this is a feature improvement that should be considered in a separate PR.

[akasunil]

@carolinan can you please review this PR and let me know your thoughts.

[carolinan]

Let's ping @amustaque97 and @ntsekouras to ask if there are any important reasons to why the spacing units was used for this font size control, that I may have missed. And @jasmussen who I believe requested that the pt size was removed. Original PR: #37311.

[jasmussen]

Thanks for the ping, that lets me clarify this part:

But one single recommedation I'd make is to adopt the unit-selector input field. That would both let us remove the (pt) from the label, and pick other units. Is that feasible?

I meant that allowing the unit selector would allow us to change the label of the previous implementation to omit the hardcoded mention of PT:

I don't personally mind if someone wants to use the pt unit for font sizes.

[ntsekouras]

Let's ping @amustaque97 and @ntsekouras to ask if there are any important reasons to why the spacing units was used for this font size control, that I may have missed.

It's been a while since that PR, but I think it's because in the block's render function(dynamic) we use wp_tag_cloud where the default unit is pt. So if we changed the default unit, it would affect all previous instances of that block..

[akasunil]

Hello, @ntsekouras I appreciate your response. Actually, the default unit is not being changed. The array of units used in the front size picker have taken the role of the spacing units settings, and the PT unit has been included in that array. PT unit support will still be available in tag cloud block.

[carolinan]

Yes the default unit should still be PT, what we are trying to solve here is that right now, on trunk, you can't switch back to PT if you select a different unit. On trunk PT is not available in the control.

[ntsekouras]

Looks good, thank you!

[carolinan]

Nik was faster :)
Thank you for working on this @akasunil. I'm glad the initial calculations were not needed.
This works well both when the theme sets spacing units with theme.json, and when it does not.

[akasunil]

Thanks @carolinan and @ntsekouras

Just wondering, though: wouldn't it be confusing if a user removed a PT unit from theme.json yet it continued to show up on the tag cloud block?

The initial calculations were to prevent this by setting the PT unit's default value to the first accessible unit's value.