-
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
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. |
@@ -96,6 +119,7 @@ export const UnitSelect = styled.select< SelectProps >` | |||
cursor: pointer; | |||
border: 1px solid transparent; | |||
height: 100%; | |||
font-family: inherit; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this explicit config because Storybook will use a radio
by default.
@@ -54,16 +51,14 @@ const DefaultTemplate: ComponentStory< typeof UnitControl > = ( { | |||
const [ value, setValue ] = useState< string | undefined >( '10px' ); | |||
|
|||
return ( | |||
<div style={ { maxWidth: '100px' } }> |
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.
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 😇 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 🚀
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:dev
Known 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)