Skip to content
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

Update InputControl and SelectControl default heights #55490

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

jameskoster
Copy link
Contributor

What?

Make the default height / min-height for InputControl and SelectControl 32px rather than 30px.

Why?

Interactive elements like buttons and inputs are trending towards a 24px, 32px, and 40px sizing scale. The current 30px default applied to InputControl and SelectControl violates this convention resulting in awkward sizing mismatches, for example in the data view pagination UI:

Screenshot 2023-10-19 at 15 00 38

The icon button(s) are 32px tall while the page number input is only 30px. This issue will likely become more apparent as we proceed with the admin redesign project.

Testing Instructions

  1. npm run storybook:dev
  2. Open Storybook (should happen automatically when the previous command completes)
  3. Observe that InputControl and SelectControl (and any components that utilise them e.g. UnitControl) are 32px tall when size is default and __next40pxDefaultSize is false.
  4. Check instances of these components in the Editor
Before After
Screenshot 2023-10-19 at 15 04 21 Screenshot 2023-10-19 at 15 04 12

@jameskoster jameskoster added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Package] Components /packages/components Needs Figma Update Needs an update to Figma for design purposes labels Oct 19, 2023
@jameskoster jameskoster requested review from ciampo and a team October 19, 2023 14:10
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely needs a changelog entry? But yes, we should not use base10 anywhere.

@github-actions
Copy link

github-actions bot commented Oct 20, 2023

Flaky tests detected in e3beb0f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6624518867
📝 Reported issues:

@ciampo
Copy link
Contributor

ciampo commented Oct 24, 2023

Relevant conversation about introducing these size changes: #55401

@jameskoster
Copy link
Contributor Author

@ciampo how do you think we should proceed here? I don't think an increase of 2px warrants a temporary prop or other affordance due to the minimal impact.

@ciampo
Copy link
Contributor

ciampo commented Oct 24, 2023

I personally agree with you — I just wanted to flag it and add more context for any future readers.

I believe this PR is good to go once unit tests are fixed

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@ciampo ciampo merged commit 39ffb68 into trunk Oct 25, 2023
50 checks passed
@ciampo ciampo deleted the update/default-control-heights branch October 25, 2023 11:27
@github-actions github-actions bot added this to the Gutenberg 17.0 milestone Oct 25, 2023
@jameskoster jameskoster added the Design System Issues related to the system of combining components according to best practices. label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design System Issues related to the system of combining components according to best practices. Needs Design Feedback Needs general design feedback. Needs Figma Update Needs an update to Figma for design purposes [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants