UnitControl: Update unit dropdown design#42000
Conversation
|
@pablohoneyhoney @jameskoster Hi 👋 Can we get some guidance on the interaction details (touch target boundary, hover/focus styles) of this unit dropdown? For reference, these are the hover/focus styles we had for the previous design: Similarly, we'll soon need guidance for the new step button designs in the NumberControl as well. (Not in this PR though) |
|
Assuming the following anatomy: Where state styling is informed by existing components ( Here's a gif demonstrating the interactions: To me this seems like a reasonable place to start that avoids changing other components (better handled separately). But I'll defer to Pablo on how to proceed. |
| cursor: pointer; | ||
| border: 1px solid transparent; | ||
| height: 100%; | ||
| font-family: inherit; |
There was a problem hiding this comment.
Similar to #38969, this was incorrectly rendering in Arial (per Chrome user agent style).
| /** | ||
| * Size of the control option. Supports "default" and "small". | ||
| * | ||
| * @default 'default' | ||
| */ | ||
| size?: SelectSize; |
There was a problem hiding this comment.
Moved to a direct import from InputControlProps so it doesn't get out of date.
|
An update to the concept above: It looks like I was using a slightly outdated Figma component with the wrong button size. We have two options for the unit button:
TertiarySmallThinking ahead to the Stepper component mentioned above, I think it makes sense to use the Tertiary variant because its dimensions match the Icon button: The only small detail we'll need to consider is the color difference: |
345a3e1 to
505c5d5
Compare
|
@jameskoster Now for the final touches ✨ Here it is with the exact same hover/focus styles as Tertiary Button: CleanShot.2022-07-23.at.08.38.10.mp4(Tertiary button has a 1px inset hover shadow, and a 1.5px outset focus shadow.) It's a bit cramped. I'll note that, at least from a technical library maintenance standpoint, we don't necessarily have to follow the Button component sizes/styles — we can easily make a new component specifically to use inside text fields. There are a few ways we can tweak the text field button, like:
Feel free to write down the styles you want, or push the changes directly to this branch (I can take care of any final refactoring that may be necessary). Known issues
|
No problem at all 😄 Any preferences on how to deal with overflows? A 24px square is pretty small and will already overflow with common units like We can for example set some kind of CleanShot.2022-07-26.at.07.07.16.mp4This means that most UnitControls will have rectangular dropdown buttons, and the proportions of those rectangles will vary depending on the longest available option in that UnitControl. That subtle variance across UnitControls may not be desirable. In that case it might be better to just set all dropdown widths to the maximum width regardless of the option lengths. |
That would be the best choice, IMO (plus maybe a conservative, safety |
|
Oh, I think it would be ok for the button to hug the current value, probably with a 24px min-width, and a max-width as @ciampo suggested. |
| size: { | ||
| control: { type: 'select' }, | ||
| }, |
There was a problem hiding this comment.
Removed this explicit config because Storybook will use a radio by default.
| const [ value, setValue ] = useState< string | undefined >( '10px' ); | ||
|
|
||
| return ( | ||
| <div style={ { maxWidth: '100px' } }> |
There was a problem hiding this comment.
Removed these max-width containers in the stories because we need to know whether the width is unconstrained by default, and we need to be able to test __unstableInputWidth.
|
Thanks y'all, I've updated the OP with a screen recording of the latest styles. This PR is now ready for final review. The Safari issue (#42650) is pretty annoying. I don't have the appetite to address it in this PR, but we might eventually need to move to a custom select implementation if we want the centered design in Safari. It's unlikely that the bug will be fixed in Safari itself — that bug report just celebrated its 12th birthday 😇 |
ciampo
left a comment
There was a problem hiding this comment.
Code changes LGTM (I just left a few minor comments)
Kapture.2022-07-26.at.18.38.33.mp4
I'll leave it up to @jameskoster for the final approval
| margin-inline-end: ${ space( 2 ) }; | ||
| padding: ${ space( 1 ) }; | ||
| color: ${ COLORS.ui.theme }; | ||
| font-size: 13px; |
There was a problem hiding this comment.
Thought: I'd love to start standardizing font-sizes across components and be able to provide a "typography palette" through the various components from the library (e.g Heading, Text..)
|
Is it possible to render the button at a width that simply hugs the selected value? At the moment width seems to be based on the longest label in the list of Something like this would be a bit better: That would enable us to have a greater max-width value as well which I think could be valuable. 48px is a bit tight. |
It will be possible if we move to a custom select implementation, which will allow us to fix the Safari issue as well. My recommendation is to merge for now with the current implementation, and move that to a follow-up issue since the task scope will be a bit big. (Unless you feel it's a blocker.)
Yes I was thinking about this too, and it will take a bit of fiddling to get the logic right. The complexity is in maintaining a reasonable flex width balance between the prefix, number, and suffix elements. For example, in a tight context like the Border control: It might even be that we'll need to allow the consumer to customize the width via props, so it makes sense in different contexts. |
|
📌 I'm going to hold off on merging this while I look into #42717 first. The unit dropdown looks fine in Storybook but completely breaks down in WP 😭 |
Yeah, I can totally see that. I would personally try to leverage flexbox and model an elastic behavior
Good catch |
I don't think so :) |
We need to know whether the width is unconstrained by default, and we need to be able to test `__unstableInputWidth`.
1ed6dfc to
aa7bf84
Compare
|
Ok, I've fixed the #42717 problem, the unit dropdown now looks the same in isolated Storybook and WP 👍 This is now ready for final review. |
ciampo
left a comment
There was a problem hiding this comment.
LGTM 🚀
We can look into refining component in follow-ups:
Is it possible to render the button at a width that simply hugs the selected value?
It will be possible if we move to a custom select implementation, which will allow us to fix the Safari issue as well. My recommendation is to merge for now with the current implementation, and move that to a follow-up issue since the task scope will be a bit big. (Unless you feel it's a blocker.)
That would enable us to have a greater max-width value as well which I think could be valuable. 48px is a bit tight.
Yes I was thinking about this too, and it will take a bit of fiddling to get the logic right. The complexity is in maintaining a reasonable flex width balance between the prefix, number, and suffix elements. For example, in a tight context like the Border control:
![]()
It might even be that we'll need to allow the consumer to customize the width via props, so it makes sense in different contexts.
|
Follow-ups moved to #42879 |















Part of #41973
What?
Updates the unit dropdown to the new designs for the 40px size variant.
Why?
To get our components closer to the mockups.
How?
Add conditional styles for the dropdown depending on the size variant.
Testing Instructions
npm run storybook:devKnown issues
Screenshots or screencast
CleanShot.2022-07-27.at.00.09.00.mp4
When the unit isn't a dropdown (i.e. single unit case)