Font size presets UI #62328
Font size presets UI #62328
Conversation
matiasbenedetto
added
[Type] Feature
|
Size Change: +1.42 kB (+0.08%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Feel free to let @WordPress/gutenberg-design know when this PR is ready to review! Since the main issue is labelled as needing design, however, I took a quick look. But I'm unsure where to look. Would you be able to add screenshots or testing instructions to the PR or issue? |
|
@WordPress/gutenberg-design here's a screencast featuring some work about this. Could you please take a look? Not all of this has already been pushed to this branch. Screencast.from.10-06-24.13.06.38.webm |
|
Adding a new font size preset: Screencast.from.10-06-24.16.43.14.webm |
|
Removing font size preset: Screencast.from.10-06-24.16.46.22.webm |
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
ℹ️ I pushed all the changes to this branch so it can be tested. |
|
Nice work, this is not that far off! I'll give this a more thorough review when I get a moment, for now I've signal boosted this a bit for feedback. Two things that do stand out to me in this part:
The "Reset" button next to the font size slider, and the Fluid yes/no. For the former, can we just remove the reset button? You can always backspace the value to clear it, or remove the font preset. For the latter, outside of better ideas, perhaps just a toggle that says "Fluid typography" rather than a yes/no control might work. |
|
This is exciting to see! It will greatly impact my work and, I'm sure, of all those creating/playing with themes as well. It would be neat if the added custom font size presets could be automatically organized by size values from Smallest > Largest so that we don't see a |
That's a nice suggestion, thanks. There's only one technical/design difficulty about that: the mix of different units. Would it be okay to order the sizes primarily by unit and secondarily by quantity? If we have a list of multiple font sizes with different units, it could look like this:
Would this type of order be OK @beafialho @jasmussen? Another possibility is to calculate the relative units with a coefficient that allows us to make an approximative scale of the real size of that font for a 'typical' desktop screen. In this example we would order by the 'result' column. The order is slightly different. It tries to replicate what would be the size of the fonts in a 'typical' (1920px wide) screen.
|
That would be ok IMO, as long as the custom ones within the same unit are ordered by sizes 👍 |
There was a problem hiding this comment.
Thanks for the great work!
My concern is that the FontSizePicker component has been significantly updated and a new component called SizeControl has also been added.
The SizeControl component is marked as private, but FontSizePicker uses it internally. Since the FontSizePicker component is public, it may indirectly become public even if the SizeControl component is private.
Do you intend for the SizeControl component to be used outside of the Global Styles?
If not, it might be better to extend the FontSizePicker component in the block-editor package rather than making changes to the components package.
I think we will need feedback from the @WordPress/gutenberg-components team if we move forward with the current approach.
| // Get the font sizes from the theme or use the default ones. | ||
| const sizes = fontSizes.theme ?? fontSizes.default; |
There was a problem hiding this comment.
Do we need to consider the case when defaultFontSizes is false?
As in this PR, I think the current component couldn't be achieved by extending the FontSizeControl component. The new SizeControl component is only a subset of the FontSizePicker extracted to be re-used independently. |
|
|
||
| export const DEFAULT_UNITS = [ 'px', 'em', 'rem', 'vw', 'vh' ]; | ||
|
|
||
| function SizeControl( props: SizeControlProps ) { |
There was a problem hiding this comment.
@WordPress/gutenberg-components how do we feel about the new SizeControl component being introduced as a separate component (a private API to start with)?
There was a problem hiding this comment.
As @jasmussen said, a big change in core components like this should be discussed in a separate issue to figure out the best approach. And the "why" of the change is not immediately clear to me from this PR — we may be able to find other approaches once the "why" is more clear.
There was a problem hiding this comment.
IMO, SizeControl shouldn't live in the @wordpress/components package, since it feels too tailored to a specific need of the editor.
We could instead discuss the need for a more general purpose UnitControl + RangeControl component, which was already initially discussed in #40507 and then abandoned for lack of priority.
There was a problem hiding this comment.
Thanks everyone for your input.
And the "why" of the change is not immediately clear to me from this PR — we may be able to find other approaches once the "why" is more clear.
@mirka I submitted the component in a new PR: #62866
IMO, SizeControl shouldn't live in the @wordpress/components package, since it feels too tailored to a specific need of the editor.
@ciampo What does it mean 'tailored for a specific need of the editor' in this context?
I don't see how SizeControl is more tailored 'for a specific need of the editor' than the FontSizePicker. Could you explain, please?
There was a problem hiding this comment.
I don't see how SizeControl is more tailored 'for a specific need of the editor' than the FontSizePicker. Could you explain, please?
That is true, and in fact I also think that FontSizePicker shouldn't be in the @wordpress/components package either for the same reason — but it is there and we can't easily remove it.
There was a problem hiding this comment.
I think the criteria for adding new components to the components package are defined here.
The components package is used by many developers. The reason I hesitate to add new components to a component package is that once a new component is exposed, it cannot be easily removed or modified, and requires maintenance work.
I feel that the versatility of the SizeControl component has not been fully evaluated so far. I think we can implement it as an internal component and move forward with this PR until the discussion progresses.
There was a problem hiding this comment.
I'm not enthusiastic about duplicating all the SizeControl logic/styles and making them live in wp-edit-site though. I wouldn't say it's a trivial component.
The possibilities I see are:
- Rework the
disableCustomFontSizesprop in FontSizePicker so that it allows for custom font sizes only (which I believe would be the equivalent of the proposed SizeControl). For example we could deprecatedisableCustomFontSizesand make a new enum prop with three options (e.g."custom" | "predefined" | "both"). - Extract to a component as proposed in Add SizeControl component #62866. For context, we do have some precedent in DateTimePicker, and I have seen a somewhat similar proposal for SizeControl already (Add SizeControl component that allows negative values & customizable range #61180). We're definitely seeing devs needing more modularity like this. In this case, I think I would prefer exporting it as a subcomponent of FontSizePicker (e.g.
FontSizePicker.CustomInput) rather than a completely separate component.
There was a problem hiding this comment.
I think the criteria for adding new components to the components package are defined here.
Yes, thanks, but I had already read that before I commented. Still, I don't see how SizeControl differs from other components available in the package and why it should not exist in the components package.
Anyway, I am not available to continue pursuing the component path anymore. It isn't the main objective of this PR to add a new component but to provide a solution for users wanting to manage font size and spacing presets. Creating the component was a side task motivated by the desire to avoid repeating code from the font size picker, but that's taking much more time than I anticipated and blocking the original objective. I'll add the UI for this inside packages/edit-site/src/components/global-styles/size-control/ and maintain the code as short as possible to make the preset management work. If there's a need for a new component, it can be added later as a follow-up. I think that will help this move forward.
There was a problem hiding this comment.
I think that this suggestion from @mirka hits the sweet spot between reducing the amount of new code introduced, future maintenance need and speed of iteration:
Rework the
disableCustomFontSizesprop inFontSizePickerso that it allows for custom font sizes only (which I believe would be the equivalent of the proposed SizeControl). For example we could deprecate disableCustomFontSizes and make a new enum prop with three options (e.g. "custom" | "predefined" | "both").
I'll see if I can spin up a PR with the necessary changes, so that this PR can use directly FontSizePicker — would that be ok @matiasbenedetto @mirka @t-hamano ?
There was a problem hiding this comment.
I think that should be a follow-up independent of this task. The component has already consumed too much time for this implementation and it should not be a blocker. Apart from that, I'm not sure modifying the FontSizePicker component is the right thing to do and it certainly doesn't work for the other similar implementation: #61986
|
@WordPress/block-themers feedback is needed here from a block theme perspective 👀 |
|
I did find it confusing that when I click on the plus icon to add a size, a size is added automatically but unlike the colors I can't edit them directly. As a developer, I would like to know if I can customize the slug when I use the UI to add sizes and then export the theme, In a separate issue @jasmussen suggested that when a control is conditional, like the settings that are only available when "fluid typography" is toggled on, the controls should always be visible but disabled by default, and enabled when the condition applies. |
There can be nuance here.
So there's an argument that both patterns can be judiciuosly applied based on works best in the given context. If we have to extract a basic rule I'd actually lean on hiding child options, including reverting my own statement from your PR, but then be sure to always indent child-options from the left, so there's a visible indication of hierarchy. Any preference? |
|
My preference would be consistency and to not have to go back and forward because it creates extra unnecessary work. |
|
I would like to receive your input about the expected behavior of the font size presets: Font size presets in the font picker and colors palette are working differently. The colors admit several 'origins' (default, theme, custom) and all of them are available to be selected by the user. Meanwhile, fonts sizes are working differently, 'theme' presets (if they are defined) overrides 'default' font sizes. 'default' presets can not be selected or seen by the user if 'theme' size presets are available. What path should we follow here? |
|
I did find it confusing, but the consistency remark was about the visibility and positioning of the conditional controls. So about the conditional controls, that are only visible depending on the parent option, it seems the common ground is to indent them. In this case, there is more than one condition, and I am not sure if it would "look good" or if translations would fit in this space if the last nested control was indented twice. |
|
I don't think there is precedence for this, but perhaps when the new size is added automatically, it could have a bit of a background color fade or similar to indicate that it is the newest item? I did not find it so easy to see which size was the one I had just added. |
@carolinan is this done for any other setting/preset? Updating the slug seems risky because all the elements where the preset was applied will point to a preset that no longer exist. |
|
As a developer I would like WP to update the slug everywhere ;) I don't want to put this on you, because you have already made progress here and probably have thought about this in detail, |
Previously, color palettes had slugs automatically generated based on the label you entered. However, if non-alphabetical characters were used in the label, the slug would not be generated correctly or would be duplicated. To solve this, a sequential number was adopted, independent of the label. See the discussion in issues below.
This rule is consistent across all of the custom color palettes, custom shadow presets, and custom font size presets in this PR. The question of whether to allow editing of the slug, or generate it by some rule based on the label, might be better discussed in a new issue covering colors, shadows, and font sizes as a whole, rather than in this PR. |
I tried both with code. They look like this.
|
Consistency is important for me because it helps me work more efficiently, both when coding the controls and using them, but yeah, I want to say that I don't like this result either. |
Yes, thank you for the background info! |
|
Seeing this here, what is the difference between "Fluid typography" and "Custom fluid values"? |
If fluid typography is enabled the default values are used for min and max. If the user want's to set manually the min and max, it should enable 'Custom fluid values'. Reference from this make post: https://make.wordpress.org/core/2022/10/03/fluid-font-sizes-in-wordpress-6-1/
|
|
Thanks for trying this. It really seems like the right solution is what you had, to hide the added controls until the toggle is flipped. There might not be a perfect solution that applies consistently for each of these cases. |
Thanks! I wonder if it might be helpful to have a description for the |
What?
Adds UI to Global styles to allow modification of font size presets.
Fixes: #61987
Why?
To allow users to edit the font size presets using the editor.
How?
By implementing the UI to handling creation, update and removal of font size presets.
Screencasts
See the comments below to find the screencasts.