CustomSelectControl: Use __unstableSize prop in Typography panel
#36162
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
Because this component is marked as experimental, I think we can default it to large.
mirka
added
[Feature] Component System
|
Size Change: +145 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
9 tasks
aaronrobertshaw
approved these changes
Nov 3, 2021
There was a problem hiding this comment.
Nice work @mirka ✨
Everything tests as advertised for me.
✅ FontSizePicker displays at appropriate size in both block and site editors
✅ FontAppearanceControl displays correctly in both editors as well
✅ Storybook updates for CustomSelectControl and TypographyPanel test well also
LGTM 👍
|
Thanks so much for working on this. I responded to your other PR about updating some of the sizes on the existing controls, it looks like this PR is intended to address that feedback? At least things feel much less jumpy here:
If so, I do like starting very conservative. For example the mix of the old size for line height and the new for font size feels off... it feels like every control inside a panel should have the same size. |
|
Thank you for the reviews!
@jasmussen Absolutely, Although I've been told that some inconsistency is expected while we transition to the new designs, I'm with you in that I want to minimize that inconsistency as much as reasonably possible. Would you prefer that we hold off merging these until the |
No no, if we have some confidence that we can land those other changes in somewhat quick order, totally fine (and good) to do it bit by bit. Thank you! |
7 tasks
7 tasks
ciampo
reviewed
Nov 24, 2021
There was a problem hiding this comment.
Just as a recap (mainly for myself, to avoid confusion):
- in Add
__unstable-largesize variant onInputControlSelectControlUnitControl#35646, we introduced one more possible value for thesizeprop, namely__unstable-large - in this PR, we're introducing a new prop called
__unstableSizeonCustomSelectControlandFontSizePicker
I'm still unsure about the variant name large. Would a more descriptive name like
sidebar-defaultorg2-defaultbe more intuitive, given the intent? Let me know what you think.
Not sure about that. We'd need to understand if we should aim at providing more standardised values for the size prop across the library (eg default, small, large), or if we want to offer more "custom" values depending on the component. Regardless, I would avoid mentioning the g2 term, as it refers to a project that ran alongside the gutenberg repo, and it could lead to more confusion for some folks.
I wonder if we should document in the respective components READMEs:
- the new
__unstable-largeprop - the new
__unstableSizeprop onCustomSelectControlandFontSizePicker
We should also add a CHANGELOG entry for the changes specific to this PR
| @@ -55,6 +55,7 @@ const stateReducer = ( | |||
| }; | |||
| export default function CustomSelectControl( { | |||
| className, | |||
| __unstableSize, | |||
There was a problem hiding this comment.
Should this have a default value of default, as suggested by the options defined in the Storybook example?
There was a problem hiding this comment.
Also, in order to align values used across components, should we also consider allowing a value of small for this prop?
| @@ -28,6 +28,7 @@ import { | |||
|
|
|||
| function FontSizePicker( | |||
| { | |||
| __unstableSize, | |||
There was a problem hiding this comment.
Should this also have a default value of default? Maybe this is fine to leave as undefined, since a default value will be potentially given by CustomSelectControl (see earlier comment)
| @@ -6,6 +6,12 @@ import CustomSelectControl from '../'; | |||
| export default { | |||
| title: 'Components/CustomSelectControl', | |||
| component: CustomSelectControl, | |||
| argTypes: { | |||
ciampo
added
Backport to WP 6.7 Beta/RC
|
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 coordinated merge with other related PRs
Description
This PR enlarges
FontSizePickerandFontAppearanceControlto the new 40px height. The height change is accomplished via a new internal__unstableSizeprop onCustomSelectControl, which is the underlying component being used.FontSizePicker(@wordpress/componentscomponent) — Because this is an already stable component, the size is enlarged only in these limited contexts:FontAppearanceControl(@wordpress/block-editorcomponent, experimental) — Because this block is still experimental and can be expected to only appear in the Typography panels, the size is enlarged by default.Background
After some talks with @jasmussen @pablohoneyhoney @griffbrad, the overall sentiment was that we weren't ready to officially commit to a new set of size variants, especially for non-experimental components (which were designed pre-G2 and are widely used in contexts other than the sidebar). For non-experimental components, it would be safer for us to keep the 40px size variant
unstableuntil we are confident to make it part of the official size variants.❓ Variant name
I'm still unsure about the variant name
large. Would a more descriptive name likesidebar-defaultorg2-defaultbe more intuitive, given the intent? Let me know what you think.How has this been tested?
Types of changes
New feature (non-breaking change which adds functionality)
TODO before merge
Checklist:
*.native.jsfiles for terms that need renaming or removal).