-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
InputControl-based components: Add opt-in prop for next 40px default size #53819
Conversation
Size Change: +69 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Flaky tests detected in 339dbc1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5907808106
|
0b96c24
to
bc60e00
Compare
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.
Thank you @mirka for working on this!
- ✅ Code changes look good
- ✅ Components on Storybook look and behave as expected (with new 40px height)
- ✅ Deprecated logic around 36px prop works as expected
- ⏳ Gave a quick look at the editor, but I could do with a few extra pairs of eyes
@WordPress/gutenberg-design @brookewp @chad1008 could you give this and check that input-based controls are looking good? (see PR description for a detailed list of what to look for).
Also, note that not all components are upsized yet — as Lena mentioned in the PR description:
Similar placeholder components in other core blocks are not upsized yet — this should be done separately.
[...]
The other components in the Advanced section are not upsized yet — this should be done separately.
One this PR is completed, we can follow up and upsize the remaining input components via the __next40pxDefaultSize
prop.
I didn't see any issues in the Editor. Excited to get everything sized consistently, I'm currently seeing inputs that are 30, 36, 40px, sometimes in the same block inspector 🙈 |
I know, right? Once this PR lands, feel free to start using the |
The next step is to ensure the same default font size and left/right padding for the compact (32px tall) button, so it becomes a just less tall version of the default 40px button. |
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.
Testing steps and smoke testing went well. It looks good to me!
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.
LGTM 🚀
The next step is to start applying the __next40PxDefaultSize
prop around the editor and make any necessary tweaks in the process.
@jameskoster , could you give us guidance on where we should start applying this prop in the editor? Or should we do it literally on every instance of these controls?
Feel free to also go ahead and apply this prop yourself — I guess you and @chad1008 can coordinate the effort together :)
The next step is to ensure the same default font size and left/right padding for the compact (32px tall) button, so it becomes a just less tall version of the default 40px button.
Hey @jasmussen , just wanted to make sure if this point is still relevant given the current implementation of Button
— if so, could you be more specific on what exactly needs to be done? Thank you!
From my POV, the plan remains #46734, which is only have two sizes, 40px (default) and 32px (small or compact), same font size, just different height. CC: @richtabor @SaxonF |
8af8b86
to
6f6b4ac
Compare
We've recently made changes to support a default 40px size (via the @chad1008 , could you add to the list of follow-ups making sure that both the compact and default size of |
Confirmed. |
Cool! Then this can happen: #55079 |
Very nice! Can I ask for TextControl next? That'll get us in a decent spot consistency wise. 😅 |
Yup, with @chad1008 we recently discussed working on |
For the dev note, see #46741 |
Part of #46741
What?
For all the InputControl-based components listed below:
__next40pxDefaultSize
prop to opt into the next 40px default size.__next36pxDefaultSize
as deprecated, and alias to the 40px prop.InputControl-based components
Silently upsized components
The following usages were upsized/tweaked to reflect the size change from 36px to 40px.
DateTimePicker / TimePicker
Block editor ▸ Position control (Add a Row block and find the Position control in the block inspector)
Block library ▸ RSS block
Similar placeholder components in other core blocks are not upsized yet — this should be done separately.
Block library ▸ Template Part ▸ Advanced ▸ Import controls (Open a template in the Site Editor, add a new template part, and see the Advanced section in the block inspector)
Testable with diff
The other components in the Advanced section are not upsized yet — this should be done separately.
Why?
We need to supersede the outdated
__next36pxDefaultSize
props and transition to the new 40px default sizes.Apologies for the relatively large surface area of this PR, since this needs to be done in one go.
How?
See code comments for noteworthy details.
Testing Instructions
npm run storybook:dev
__next36pxDefaultSize
has a defined value, there should be a deprecation warning in the console. When it's truthy, it should work the same as__next40pxDefaultSize
(i.e. should have 40px height and not 36px).__next40pxDefaultSize
is falsey, it should have no effect on the existing size variations. When it's truthy, it should make thedefault
size variant be equivalent to the__unstable-large
variant.