TextareaControl: Update styles #64586
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
mirka
added
[Type] Enhancement
|
Size Change: -3 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
@WordPress/gutenberg-design The specs define a 10px vertical padding, but I tweaked it to 11.5px so the control height matches a standard InputControl when the TextareaControl is set to show one line of text only (set the 
Also, this upsizing on TextareaControl is unlikely to seriously break consumer usage, so I think we could get away with just shipping this change without first hiding it behind an opt-in prop. If there are no particular objections here, I'll go with that. (cc @WordPress/gutenberg-components) |
28 tasks
|
Flaky tests detected in 666f6ea. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10426397094
|
|
Great improvement, thank you.
Just to be sure, which specs are these? At a glance, this looks like a solid win for consistency, and that what exists in trunk is simly vestigial and overlooked. There's a lot of green in the code, so I'll defer to a code review for what's best there, but designwise, this looks great. |
|
@jasmussen Thanks for confirming! I was referring to the specs here. |
|
Right, those definitely seem a bit out of date. I'll add the "Needs Figma Update" label here, but let's also make sure to get this right in the new file. I think the only downside: 11.5 is so close to 12px which would be a nice round number. I guess it's the fact that most inputs are 40px, but the 13px font centered inside with its default lineheight that's dictating this odd number yes? Thanks for diving in! |
tyxla
reviewed
Aug 19, 2024
|
It's not just about single-line textareas, but also ensuring the copy inside sits on the same baseline as regular text inputs, like this: 
I think it can be achieved by updating the following textarea properties:
In this case using a padding value off the 4px baseline seems unavoidable because the input height is 40, minus the line-height (20) equals 20, divide by 2 = 10. |
|
Thank you for the design guidance! Here is the result, baseline matched with the other similar components: 
I also wanted to mention a nuance with the Figma metrics. Not sure if this is fully consistent across the file, but at least for the one I'm looking at, the stroke is inset so the distance includes the border width: 
In code, it kind of depends on the implementation whether the border is "included" or not. In TextareaControl, the border is not included so we need to subtract the border widths from the actual padding values. For InputControl-based components on the other hand, the border is rendered separately from the actual |
ciampo
approved these changes
Aug 20, 2024
packages/components/src/textarea-control/styles/textarea-control-styles.ts
Outdated
Show resolved
Hide resolved
mirka
changed the title
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Ok, I updated the PR description and screenshots. This is good to go on my end if we're happy with the changes 👍 |
Makes sense. Is there a reason not to include the spacing scale in For similar components, code like |
tyxla
approved these changes
Aug 20, 2024
There was a problem hiding this comment.
LGTM 👍 🚀
@jameskoster's suggestions on extracting some spacing vars make sense, but I think they can also be followed up with in another PR as needed.
|
I'll probably do some value abstracting when I update the input padding values from 16px to 12px (#64520 (comment)) 👍 |
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
* TextareaControl: Refactor styles * Add 40px size prop * Tweak typography styles * Clarify with comment * Remove opt-in prop * Improve comment * Clean up unnecessary prop type * Add changelog Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
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 #46741
What?
Update styles on TextareaControl to match the current specs. Namely, increased padding and leading.
Testing Instructions
See Storybook for TextareaControl. See usages in the editor.
Note for code review: The first commit is simply a move of the styles in
utils/input/which were only used for this TextareaControl component. The effective changeset is much smaller.Screenshots or screencast
How it looks in context with other controls:
Or in this Additional CSS field: