Conversation
| * The `InputLayout.Slot` component can be used to add standard padding in | ||
| * the `prefix` or `suffix` slot. | ||
| * | ||
| * The `padding="minimal"` setting will work best when the slot content is a button or icon. |
There was a problem hiding this comment.
I'll add a proper story for this "minimal" padding case after we add an IconButton component.
|
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. |
|
Size Change: 0 B Total Size: 3.05 MB ℹ️ View Unchanged
|
|
Flaky tests detected in fc024b9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20853716723
|
| className={ clsx( | ||
| styles[ 'input-layout-slot' ], | ||
| styles[ `is-${ type }` ], | ||
| padding !== 'default' && styles[ `is-padding-${ padding }` ] |
There was a problem hiding this comment.
More of a general comment on "default" values, but this feels like either:
- If we don't want the default to effect some state, then it should be
undefinedby default. - Otherwise, we should remove the condition and let the class be applied.
My preference here would be toward the first. But I think we'll want to adopt a consistent and documented approach across our components.
| padding !== 'default' && styles[ `is-padding-${ padding }` ] | |
| padding && styles[ `is-padding-${ padding }` ] |
There was a problem hiding this comment.
API-wise, apart from early components like Button which were before our time, we've tried to be consistent about having an explicit "default" value for size and variant props. Although it can feel verbose in code, I think this is the best for consumer clarity — there's no room for confusion about what the default is, and it's clearer to talk about because it matches human language ("the default variant" vs "the undefined variant").
Assuming we'll continue this pattern, do you still prefer a class to be applied? I feel like classes are just an implementation detail that may or may not be necessary for any specific style implementation, especially with CSS modules where they are going to be mangled in the final output anyway. We are actually free to do styles[ 'is-padding-${ padding }' ] with no condition at all, just that we'd be adding a technically unnecessary class in the default case. So, no strong opinion there, but wanted to understand your reasoning.
There was a problem hiding this comment.
Yeah, reflecting on my comment, I think I was considering this in the context of consistency in how we consider the default value. If it's valuable to be explicit with a default 'default' value, then we should treat it the same as any other valid state for that value, i.e. apply a 'is-padding-default' class. Or at least not go out of our way to add logic to handle it distinctly.
But while I think it's good to get on the same page with how we consider defaults (should probably document), the specific point of whether or not to apply the class is pretty negligible and I don't have strong feelings.
There was a problem hiding this comment.
Gotcha. I'm fine with removing the logic, if only for simplicity 👍 2e17b3b
|
|
||
| @layer wp-ui-components { | ||
| .input-layout { | ||
| --wp-ui-input-layout-padding-inline: var(--wpds-dimension-gap-sm); |
There was a problem hiding this comment.
This feels to me like it should use a "padding" token. We probably want to create a separate set of padding tokens for interactive / form elements. It might end up being the same value as --wpds-dimension-gap-sm, but I think conceptually this represents something different.
There was a problem hiding this comment.
Good point, this is padding and not a gap.
Any reason we should have a separate set of padding tokens for interactive/form elements, rather than expand the existing set to cover it?
@WordPress/gutenberg-design The Input and Select components use an inline padding value of 12px, but this value is not included in the dimension-padding-surface tokens. How do you prefer we add it?
(I'll revert this PR to use a multiple of dimension-base, and we can deal with the tokens separately.)
There was a problem hiding this comment.
My reason for suggesting a separate set was that the existing set is tailored for "surface" targets, where with other token values (color) we've been categorizing input aspects under "interactive".
There might be an argument that we don't need these targets at all for paddings and can be flattened to a uniform set of padding tokens (like what was decided for gaps), though I'd defer to design folks whether that's something we want to be able to differentiate.
A bit of past discussion in #72984 (comment)
There was a problem hiding this comment.
Left some general thoughts about dimension tokens and density here
| font-size: 16px; /* for mobile */ | ||
| line-height: 1; | ||
| color: var(--wpds-color-fg-content-neutral); | ||
|
|
||
| @media (min-width: 600px) { | ||
| font-size: 13px; |
There was a problem hiding this comment.
Should we use font tokens? Also noting with the breakpoint this is something we'll want to consider as part of #73361. Regarding the need for mobile minimum font size, maybe we could make it at least somewhat token-compliant with max as a way of communicating "we want to make sure it's at least 16px due to mobile behaviors" ?
| font-size: 16px; /* for mobile */ | |
| line-height: 1; | |
| color: var(--wpds-color-fg-content-neutral); | |
| @media (min-width: 600px) { | |
| font-size: 13px; | |
| font-size: max(var(--wpds-font-size-md), 16px); /* avoid mobile zoom */ | |
| line-height: 1; | |
| color: var(--wpds-color-fg-content-neutral); | |
| @media (min-width: 600px) { | |
| font-size: var(--wpds-font-size-md); |
There was a problem hiding this comment.
I love the max idea. Great way to make absolute values work with tokens.
# Conflicts: # packages/ui/CHANGELOG.md # packages/ui/src/form/primitives/index.ts
| font-family: var(--wpds-font-family-body); | ||
| font-size: max(var(--wpds-font-size-md), 16px); /* avoid mobile zoom */ | ||
| line-height: 1; | ||
| color: var(--wpds-color-fg-content-neutral); |
There was a problem hiding this comment.
I think this should be interactive.
There was a problem hiding this comment.
Actually, wait, does this apply to the input or just the prefix/suffix? If it's the latter ignore me :)
There was a problem hiding this comment.
With that said, I think it might be beneficial to use --wpds-color-fg-content-neutral-weak for the prefix/suffix to better distinguish it from the input value.
There was a problem hiding this comment.
With that said, I think it might be beneficial to use
--wpds-color-fg-content-neutral-weakfor the prefix/suffix to better distinguish it from the input value.
Interesting. I do see some potential drawbacks there, e.g. looking placeholder-like (and thus clickable) or disabled. Maybe best to leave alone for now. And consumers can always change the color if desired.
There was a problem hiding this comment.
Ok, I'll try this in a follow-up PR (there are additional things I need to test) 👍
aduth
left a comment
There was a problem hiding this comment.
Needs a rebase, and I left a couple non-blocking comments. LGTM otherwise 👍
| --wp-ui-input-layout-padding-inline: calc(var(--wpds-dimension-base) * 3); | ||
|
|
||
| display: flex; | ||
| height: 40px; |
There was a problem hiding this comment.
Similar to what was discussed in #73875 (specifically #73875 (comment)), do we need an explicit height, or could we achieve this through a combination of padding, line-height, font-size, etc. Or at least, if we needed an explicit height, would it be better to express in this in terms of what we're trying to do with it (i.e. align to some 4px grid, based on --wpds-dimension-base).
There was a problem hiding this comment.
Valid questions. Let's figure this out as part of #74556. Part of it is understanding what exactly is needed from size/density theming, from a product standpoint. Do form components really need to scale fluidly? That kind of thing.
|
|
||
| @layer wp-ui-components { | ||
| .input-layout { | ||
| /* TODO: Use padding tokens */ |
There was a problem hiding this comment.
Can we create and link to a related issue? Or will this be a quick follow-on?
There was a problem hiding this comment.
I don't expect this to be a quick decision 😅 I'll link to the issue Marco posted for us #74556.
# Conflicts: # packages/ui/CHANGELOG.md

Part of #74178
What?
Add
InputLayoutprimitive component to@wordpress/ui.Why?
This is a presentational (visual design, layout) component with no semantics. It's used to provide consistent styling and
size/prefix/suffixAPIs to similarly styled components likeInputandSelect.The equivalent in
@wordpress/componentswould be theInputBasecomponent (internal only).Testing Instructions
See stories in Storybook.
Screenshots or screencast