FontSizePicker: Update hint text to match the design #44966
Merged
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
noisysocks
added
[Type] Enhancement
|
Size Change: +99 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
This was referenced Oct 25, 2022
noisysocks
force-pushed
the
update/font-size-picker-header-hint
branch
from
64d696d
to
26a8a20
Compare
noisysocks
changed the base branch from
trunk
to
update/simplify-font-size-picker
|
I stacked this onto #44891. |
noisysocks
force-pushed
the
update/font-size-picker-header-hint
branch
from
d2f6713
to
86bff2b
Compare
noisysocks
commented
Nov 2, 2022
| @@ -79,6 +79,7 @@ export default function CustomSelectControl( props ) { | |||
| value: _selectedItem, | |||
| onMouseOver, | |||
| onMouseOut, | |||
| __experimentalShowSelectedHint = false, | |||
There was a problem hiding this comment.
I obviously don't love adding yet another prop 😅
We've talked before about introducing renderItem or something like that which should negate the need for this. I figure let's add the dumb prop for now and come back to this if/when we pick #41726 back up. CustomSelectControl is in need of a lot of love!
There was a problem hiding this comment.
This looks like a reasonable compromise that we can remove once we're done reworking CustomSelectControl. It should be ok to remove this prop in the near future before it makes its way into WordPress core.
CustomSelectControlis in need of a lot of love!
We are well aware of it! Unfortunately our low capacity and some unforeseen high priority tasks (i.e. fixing the high amount of regressions introduced by the Popover refactor) caused some delays. We should be able to resume work on it very soon (maybe we can even start collaborating on it next week, if we find some time)
There was a problem hiding this comment.
Just a quick request — can we add its associated control in Storybook?
diff --git a/packages/components/src/custom-select-control/stories/index.js b/packages/components/src/custom-select-control/stories/index.js
index 585c380923..4891bcf210 100644
--- a/packages/components/src/custom-select-control/stories/index.js
+++ b/packages/components/src/custom-select-control/stories/index.js
@@ -8,6 +8,7 @@ export default {
component: CustomSelectControl,
argTypes: {
__next36pxDefaultSize: { control: { type: 'boolean' } },
+ __experimentalShowSelectedHint: { control: { type: 'boolean' } },
size: {
control: {
type: 'radio',There was a problem hiding this comment.
(maybe we can even start collaborating on it next week, if we find some time)
Let's!
ramonjd
approved these changes
Nov 2, 2022
There was a problem hiding this comment.
Testing well for me. Code looks good, but will defer to components experts on the finer points.
✅ Units don't show on the options control
✅ Custom, not (Custom)
✅ Mixed units show on the select control
✅ For same units, the unit is shown in the label
Just for the record, with fluid typography enabled and > 5 fontSizes the post editor does display the size values in the dropdown select menu, while the site editor does not.
I can't remember but I guess it's due to the way we store the value in global styles.
Site editor:
Post editor:
ciampo
approved these changes
Nov 2, 2022
There was a problem hiding this comment.
Code changes look good and make sense.
If we need to land this without waiting for the CustomSelectControl refactor, then adding the __experimentalShowSelectedHint prop is necessary — we'll just need to make sure that we'll remove it as soon as CustomSelectControl supports rendering custom markup for its options.
Just for the record, with fluid typography enabled and > 5 fontSizes the post editor does display the size values in the dropdown select menu, while the site editor does not.
I can't remember but I guess it's due to the way we store the value in global styles.
It would be interesting to understand why this is happening. Is it a regression that can be associated to a PR in particular? Or has this always been the case for fluid typography?
noisysocks
force-pushed
the
update/font-size-picker-header-hint
branch
from
86bff2b
to
07c4151
Compare
|
Thanks!
Yeah, I don't think we should wait for I'll make a note somewhere to come back to this.
It looks like this happens in |
I'll test this again once #44967 is rebased (today sometime) |
Just following up on this. It's indeed because the font size picker is receiving In the post editor:
In the site editor:
I'm convinced we can fix it as part of #44967. This is because that PR ensures we use the preset CSS var to set the style rather than a clampified value. Seems to work!
|
62 tasks
14 tasks
34 tasks
11 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?
Updates the header hint text (shown next to the Size label) and select hint text (shown alongside each option in the select dropdown) in
FontSizePickerto match what's in the design.Why?
Part of a focus into making the Typography panel look like it does in the design. See #34345 (comment).
How?
Here's the logic that I deduced from the design:
Header hint text:
Select hint text:
clamp()), display it; otherwiseCustomSelectControlhas been updated to allow showing the selected hint text in the button per the designs.Testing Instructions
settings.typography.fontSizesin your theme.json.Screenshots or screencast
5 sizes or fewer:
Custom value:
More than 5 sizes – same unit:
More than 5 sizes – different units: