LineHeightControl: Use __unstableSize prop in Typography panel
#36196
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
mirka
added
[Package] Block editor
9 tasks
|
Size Change: +297 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
|
Thanks so much for starting work on this. It's so important that we keep up morale and momentum on the components effort, lest it will appear as if we can't make any progress on it at all. Now I know that this one is meant to be paired with #36162, but it does make it clear that there are perhaps, worms, in this can: This screenshot surfaces that suddenly the letter case control feels off the grid. Going back to #27331 and notably the type panel (I know that should be using 40px instead of 32), it shows how all of the controls, input or otherwise, are the same height. Even if we can move quickly (and you are, thank you!) it still suggests the limitation of doing these one by one. I wonder if there's a hacky way we could do this. If the following is a crazy idea do dismiss it, but just so it's stated:
Would that, or something like it, work? |
| @@ -5,4 +5,14 @@ | |||
| display: block; | |||
| max-width: 60px; | |||
| } | |||
|
|
|||
| // TODO: This may be easier if we can replace TextControl with NumberControl and use prop `size="__unstable-large"` | |||
There was a problem hiding this comment.
I think you make a good point here. The fact that this control is also not a NumberControl means you can't drag up/down like most other number related inputs. This catches me out a lot and has also come up in a few team chats and reviews of the typography panel previously.
Quickly switching out the TextControl for the NumberControl appeared to work fine for me.
There was a problem hiding this comment.
I'd be definitely up for switching to using NumberControl here — not only if makes sense in terms on semantics and UX, but the code would also look cleaner.
Regarding using experimental components inside stable components, I'm generally ok with that, especially if the experimental component remains as an implementation detail of the stable component, that can be swapped/changed without introducing breaking changes to the public APIs of the stable component.
Alternatively, I'd try to see if we can apply the "size" normalisation to the TextControl component in the @wordpress/components package, rather than doing in in the block editor
7 tasks
|
Both your sharp eyes have picked up on things I hadn't realized — really appreciate the great reviews! So based on @jasmussen's feedback, here's a tracking issue with the revised plan 👉 #36230 I thought it'd be a nice touch if we could land the new sizes in WP 5.9, but now that I see that was a bit unrealistic, there's no need to rush. With the revised plan I think we can upsize all the Typography panel elements "at once" without much overhead (like cleaning up temporary variables or rebase hell). |
|
Thanks for all your careful work. I would agree, with three releasees per year, there's no need to rush it, and it's important to get it right. |
ciampo
added
[Type] Enhancement
ciampo
reviewed
Nov 24, 2021
| @@ -5,4 +5,14 @@ | |||
| display: block; | |||
| max-width: 60px; | |||
| } | |||
|
|
|||
| // TODO: This may be easier if we can replace TextControl with NumberControl and use prop `size="__unstable-large"` | |||
There was a problem hiding this comment.
I'd be definitely up for switching to using NumberControl here — not only if makes sense in terms on semantics and UX, but the code would also look cleaner.
Regarding using experimental components inside stable components, I'm generally ok with that, especially if the experimental component remains as an implementation detail of the stable component, that can be swapped/changed without introducing breaking changes to the public APIs of the stable component.
Alternatively, I'd try to see if we can apply the "size" normalisation to the TextControl component in the @wordpress/components package, rather than doing in in the block editor
This was referenced Dec 6, 2021
|
Closing in favor of the approach taken in #42718 |
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.
Part of #36230
🚧 On hold until #37110 is done
Description
Enlarges
LineHeightControlto the new 40px height in the Typography panels only.The height change is accomplished via a new internal
__unstableSizeprop.How has this been tested?
Screenshots
@jasmussen Do you mind that the max-width is removed in the Site Editor version? I think that panel is meant to be replaced with a
ToolsPanelimplementation, so eventually we'll be using the "single column" grid layout there to bring it in line with the block editor Typography panel.Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).