-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Adding Font size presets UI #63057
base: trunk
Are you sure you want to change the base?
Adding Font size presets UI #63057
Conversation
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. |
Size Change: +1.96 kB (+0.11%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
It looks amazing :) Good job @matiasbenedetto 👍 One question: Couldn't this component be used for margins and paddings too, so they can be adjusted centrally? |
@BenjaminZekavica do you mean implementing this kind of UI to manage the spacing presets? |
Took this one for a spin. It's close! I'm not sure if all of the following feedback has to be addressed in this PR, probably much of it are existing issues. But we should circle back to fix these, they are starting to compound. One issue: This is using the "ItemGroup" pattern. Those should always be 40px tall, like buttons. This one is ~44px. CC: @richtabor as I know you've worked on an adjacent palette drilldown, in case there's some DNA we can share. If the heading should match from ItemGroup to drilldown title, it should say "Font size presets" (note, no number of presets, and fix the typo that says "Font sizes presets". Depending on what Rich says, though, we might want to change it to "Edit size presets". Similarly, these itemgroup items are ~32px, and should also be 40px: A few more things:
There's still some curious focus style issue for itemgroup items: When I open the modal to rename a preset, focus is on the dialog itself: Can you set focus on the input field? Then it will match similar rename dialogs elsewhere in the UI: This option could use an "Are you sure" confirm: For the individual font sizes:
A possible followup to consider; if you set a min-value that's larger than the max value, nothing happens, and the max value is used. Perhaps we can be smarter? Or maybe it's fine? With that, I think we can land this one. |
@jasmussen Thanks for the review! I implemented all your suggestions. I left out only 2 items:
This seems related to the styles of that component. This PR doesn't add or modify the styles of that component so I consider this item out of the scope of this PR.
Make the controls smarter seems good for a follow-up. I think it should not block this implementation. |
I removed the large prop, but the tall of the ItemGroup component seems out of the scope of this PR because it wasn't changed in any way in this implementation and I think it should not block it. |
It seems like the base size is ignore when a min a max is set so it would good to add some kind of visual hint about that. |
selectedBlockClientId, | ||
selectedBlockName, | ||
blockHasGlobalStyles, | ||
navigator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new dependency on navigator
is causing the push-to-global-styles
test to fail. The test expects to need to click the "Headings" button in the Styles panel, but the Styles panel path is now updated whenever the selected block changes, which means the Headings panel is opened automatically when a Heading block is selected.
This saves a click, but I'm not sure what the intended behaviour is. Either:
- The Styles panel path should update whenever the selected block changes - saves on clicks, but is different to the current behaviour.
- The Styles panel path does not change regardless of the selected block, and it will only change with direct interaction (i.e. navigation) within the Styles panel.
If it's number 1 we should update the test to remove these additional navigation clicks; if it's number 2 then I think we should remove the dependency on navigator
here.
Also an additional nuance, it appears that the font size in the preview takes from the base size, not the min/max values. So beyond any additional ideas coming in, can we:
|
Thanks for your continued efforts. Some of these were mentioned in the previous PR, but after some quick testing I noticed the following: 1. The font size value is not fully displayed even though the label is short.
2. Default/Theme palettes can be renamed and edited. No such specification exists for color or shadow palettes. I think a condition like this for a shadow palette is necessary. |
const handleConfirm = async () => { | ||
toggleOpen(); | ||
handleRemoveFontSize( fontSize ); | ||
navigator.goTo( '/typography/font-sizes/' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need navitator.goTo()
because we have goBack()
here.
<Button | ||
size="small" | ||
icon={ moreVertical } | ||
label={ __( 'Menu' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label={ __( 'Menu' ) } | |
label={ __( 'Font size options' ) } |
Improved accessibility. There might be a more appropriate label.
<Button | ||
size="small" | ||
icon={ moreVertical } | ||
label={ __( 'Menu' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label={ __( 'Menu' ) } | |
label={ __( 'Font size presets options' ) } |
<RenameFontSizeDialog | ||
fontSize={ fontSize } | ||
isOpen={ isRenameDialogOpen } | ||
toggleOpen={ toggleRenameDialog } | ||
handleRename={ handleNameChange } | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<RenameFontSizeDialog | |
fontSize={ fontSize } | |
isOpen={ isRenameDialogOpen } | |
toggleOpen={ toggleRenameDialog } | |
handleRename={ handleNameChange } | |
/> | |
<RenameFontSizeDialog | |
fontSize={ fontSize } | |
toggleOpen={ toggleRenameDialog } | |
handleRename={ handleNameChange } | |
/> |
The RenameFontSizeDialog
component does not receive an isOpen
prop.
<InputControl | ||
autoComplete="off" | ||
value={ newName } | ||
onChange={ setNewName } | ||
label={ __( 'Name' ) } | ||
placeholder={ __( 'Font size preset name' ) } | ||
/> | ||
<Spacer marginBottom={ 6 } /> | ||
<Flex justify="flex-end" expanded={ false }> | ||
<FlexItem> | ||
<Button variant="tertiary" onClick={ toggleOpen }> | ||
{ __( 'Cancel' ) } | ||
</Button> | ||
</FlexItem> | ||
<FlexItem> | ||
<Button variant="primary" type="submit"> | ||
{ __( 'Save' ) } | ||
</Button> | ||
</FlexItem> | ||
</Flex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<InputControl | |
autoComplete="off" | |
value={ newName } | |
onChange={ setNewName } | |
label={ __( 'Name' ) } | |
placeholder={ __( 'Font size preset name' ) } | |
/> | |
<Spacer marginBottom={ 6 } /> | |
<Flex justify="flex-end" expanded={ false }> | |
<FlexItem> | |
<Button variant="tertiary" onClick={ toggleOpen }> | |
{ __( 'Cancel' ) } | |
</Button> | |
</FlexItem> | |
<FlexItem> | |
<Button variant="primary" type="submit"> | |
{ __( 'Save' ) } | |
</Button> | |
</FlexItem> | |
</Flex> | |
<VStack spacing="3"> | |
<InputControl | |
__next40pxDefaultSize | |
autoComplete="off" | |
value={ newName } | |
onChange={ setNewName } | |
label={ __( 'Name' ) } | |
placeholder={ __( 'Font size preset name' ) } | |
/> | |
<HStack justify="right"> | |
<Button | |
__next40pxDefaultSize | |
variant="tertiary" | |
onClick={ toggleOpen } | |
> | |
{ __( 'Cancel' ) } | |
</Button> | |
<Button | |
__next40pxDefaultSize | |
variant="primary" | |
type="submit" | |
> | |
{ __( 'Save' ) } | |
</Button> | |
</HStack> | |
</VStack> |
Make it more consistent with other rename UIs
Before | After |
---|---|
![]() |
![]() |
|
||
// Receives the new value from the UnitControl component as a string containing the value and unit. | ||
const handleUnitControlChange = ( newValue ) => { | ||
onChange?.( newValue ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the ?
unnecessary?
What?
Adds UI to Global styles to allow modification of font size presets.
Fixes: #61987
This implementation started here #62328 but the approach used changed so I'm continuing that work in this new PR.
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.
Testing Instructions
Watch the screencast to see how the UI works and try to use the feature on your testing env.
Screenshots or screencast
Screencast.from.02-07-24.17.45.39.webm
📝 Reminder:
Before merging add the props from the first PR: