Adding Font size presets UI #63057
Merged
Adding Font size presets UI #63057
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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: -4.02 kB (-0.23%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
matiasbenedetto
added
[Type] Feature
Closed
|
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. |
|
This is getting close. Here's a status:
A question:
What happens to the base "Size" when you add custom fluid values? The preview appears to still render from the base size. This may become a rabbit hole, but I wonder if the base size panel should be grayed and say "Fluid" in italic" inside the disabled field, when custom values are added? Kind of like how Image dimensions say "auto"? Would appreciate a gut-check by @WordPress/gutenberg-design on this one. These items are now 49px tall:
As I noted in another PR, every ItemGroup should be exactly 40px tall, unless we're looking at edgecases where the text wraps. I also noted that this may be a separate issue. The padding left and right is also larger than in other ItemGroup Items. But in case you added a "large" prop here, best remove it at least. A way to think of it is that we are creating a typography counterpart to this palette screen:
The last piece is this one, and I'd also love additional input on this: IMO it should say either "Font size presets" (without a number), or it should say "Edit size presets" (also without a number). The former matches the drilldown page title, the latter is more active language. If there's broad support for including the number, then it should be "6 font size presets" (lowercase f), but let's hear from @WordPress/gutenberg-design. These all seem small, so nice work, this is very close! Agreed many of the feedback items can be separate. |
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. |
@jasmussen, yes! added here:
Yes, done here: |
Co-authored-by: Aki Hamano <[email protected]>
t-hamano
reviewed
Jul 11, 2024
packages/edit-site/src/components/global-styles/font-sizes/font-size.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/size-control/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/size-control/index.js
Outdated
Show resolved
Hide resolved
Would this be possible to solve the issue by making the following changes? diffdiff --git a/packages/edit-site/src/components/global-styles/font-sizes/font-sizes.js b/packages/edit-site/src/components/global-styles/font-sizes/font-sizes.js
index f0f9f7307b..a95bc32cd8 100644
--- a/packages/edit-site/src/components/global-styles/font-sizes/font-sizes.js
+++ b/packages/edit-site/src/components/global-styles/font-sizes/font-sizes.js
@@ -10,6 +10,7 @@ import {
__experimentalItemGroup as ItemGroup,
__experimentalVStack as VStack,
__experimentalHStack as HStack,
+ FlexBlock,
FlexItem,
Button,
} from '@wordpress/components';
@@ -89,7 +90,9 @@ function FontSizeGroup( {
<Button
size="small"
icon={ moreVertical }
- label={ __( 'Font size presets options' ) }
+ label={ __(
+ 'Font size presets options'
+ ) }
/>
}
>
@@ -113,9 +116,9 @@ function FontSizeGroup( {
path={ `/typography/font-sizes/${ origin }/${ size.slug }` }
>
<HStack direction="row">
- <FlexItem className="edit-site-font-size__item">
+ <FlexBlock className="edit-site-font-size__item">
{ size.name }
- </FlexItem>
+ </FlexBlock>
<FlexItem>
<HStack justify="flex-end">
<FlexItem className="edit-site-font-size__item edit-site-font-size__item-value">
diff --git a/packages/edit-site/src/components/global-styles/style.scss b/packages/edit-site/src/components/global-styles/style.scss
index d8f1c0b7c8..8fa66b6d1b 100644
--- a/packages/edit-site/src/components/global-styles/style.scss
+++ b/packages/edit-site/src/components/global-styles/style.scss
@@ -31,7 +31,6 @@
}
.edit-site-font-size__item-value {
- max-width: 50% !important;
color: $gray-700;
}
|
Good, yes that fixes the problem. @t-hamano Feel free to commit directly next time. |
|
Thanks for the reviews. I think I addressed all the feedback received. |
mikachan
approved these changes
Jul 11, 2024
There was a problem hiding this comment.
Thanks for all the work on this, @matiasbenedetto 👏
This is testing great for me:
- Can see all font size presets from the current theme
- Can add and delete custom presets
- The font sizing controls all work as I expect
- Font size presets are successfully saved in theme.json via CBT plugin (and therefore must be saved successfully in global styles), and appear under 'Theme' rather than 'Custom' after saving to the theme
I also looked through the previous feedback and I can't see anything that wasn't addressed 👍
This looks good to me, although it might be good to get one more 👀 from @WordPress/gutenberg-design before bringing this in.
|
Working well for me in a quick test, and well addressed past feedback. Thanks for working on this! |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
* adding ui for size presets * add disable prop * removes font size custom ordering * use origins * fix spelling * fixing font size presets count button size * size of font size preset item * edit wording * style on font size preset item * focus on input on the rename preset modal * redirect to font sizes * Add confirm dialog to reset font size presets * update wording * remove size prop for ItemGroup * remove number in font size presets drilldown * improve item look * update label Co-authored-by: Aki Hamano <[email protected]> * make markup more consistent with other rename dialogs Co-authored-by: Aki Hamano <[email protected]> * update label Co-authored-by: Aki Hamano <[email protected]> * disalow rename and delete of default font size presets * add missing imports * disable font main size component when custom fluid is set * use getComputedFluidTypographyValue to compute the size of the preview * remove not needed prop Co-authored-by: Aki Hamano <[email protected]> * remove not needed prop Co-authored-by: Aki Hamano <[email protected]> * allow to rename and delete only custom font size presets * improve styles * remove not needed classes * revert unwated changes * removing not needed navigation * removing not needed condition * improve array comparison --------- Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: mirka <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: beafialho <[email protected]> Co-authored-by: markhowellsmead <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: luminuu <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: BenjaminZekavica <[email protected]>
2 tasks
fabiankaegy
added
the
Needs Dev Note
97 tasks
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: