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

UnitControl: Update unit dropdown design #42000

Merged
merged 11 commits into from
Aug 2, 2022
Merged

Conversation

mirka
Copy link
Member

@mirka mirka commented Jun 28, 2022

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

  1. npm run storybook:dev
  2. See the stories for UnitControl.

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)

Dropdown text has no accent color when the dropdown is inactive

@mirka mirka added the [Package] Components /packages/components label Jun 28, 2022
@mirka mirka self-assigned this Jun 28, 2022
@mirka
Copy link
Member Author

mirka commented Jun 28, 2022

@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:

Hover style Focus style

Similarly, we'll soon need guidance for the new step button designs in the NumberControl as well. (Not in this PR though)

image

@jameskoster
Copy link
Contributor

Assuming the following anatomy:

Screenshot 2022-06-28 at 12 11 49

Where state styling is informed by existing components (InputControl and Button), we'd end up with the following:

Screenshot 2022-06-28 at 12 14 18

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;
Copy link
Member Author

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).

Comment on lines -57 to -62
/**
* Size of the control option. Supports "default" and "small".
*
* @default 'default'
*/
size?: SelectSize;
Copy link
Member Author

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.

@jameskoster
Copy link
Contributor

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:

  1. Use the Tertiary button variant (36px height)
  2. Use the Small button size (24px height)

Tertiary

Small

Thinking ahead to the Stepper component mentioned above, I think it makes sense to use the Tertiary variant because its dimensions match the Icon button:

Screenshot 2022-06-29 at 11 06 29

The only small detail we'll need to consider is the color difference:

Screenshot 2022-06-29 at 11 08 57

@mirka mirka force-pushed the update/unit-control-dropdown branch from 345a3e1 to 505c5d5 Compare July 22, 2022 23:40
@mirka
Copy link
Member Author

mirka commented Jul 23, 2022

@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:

  • Make it slightly smaller than 36px.
  • Change the hover/focus styles.

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

@jameskoster
Copy link
Contributor

Hmm yeah that is a bit tight isn't it. Especially hovering the unit button after focussing the input.

Looking at the 'Remove color' button from global styles (which is somewhat similar in terms of general UI) we can probably try small icon Buttons:

Screenshot 2022-07-25 at 10 37 26

Sorry for flip-flopping 😓

@mirka
Copy link
Member Author

mirka commented Jul 25, 2022

Sorry for flip-flopping 😓

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 em/rem.

Overflowing with 'em' unit

We can for example set some kind of max-width, and grow to the longest option label within that max.

CleanShot.2022-07-26.at.07.07.16.mp4

This 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.

@ciampo
Copy link
Contributor

ciampo commented Jul 26, 2022

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 max-width in case the unit label is extremely long?)

@jameskoster
Copy link
Contributor

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.

Comment on lines -27 to -29
size: {
control: { type: 'select' },
},
Copy link
Member Author

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' } }>
Copy link
Member Author

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.

@mirka mirka marked this pull request as ready for review July 26, 2022 15:17
@mirka mirka requested a review from ajitbohra as a code owner July 26, 2022 15:17
@mirka
Copy link
Member Author

mirka commented Jul 26, 2022

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 😇

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.

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;
Copy link
Contributor

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..)

@jameskoster
Copy link
Contributor

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 options, which can have some awkward manifestations when there's a mixture of short/long labels:

Screenshot 2022-07-27 at 12 49 36

Something like this would be a bit better:

Screenshot 2022-07-27 at 12 51 35

That would enable us to have a greater max-width value as well which I think could be valuable. 48px is a bit tight.

@mirka
Copy link
Member Author

mirka commented Jul 27, 2022

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:

A BorderControl field that has a prefix, number field, and unit dropdown all in a 110px width

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.

@mirka
Copy link
Member Author

mirka commented Jul 27, 2022

📌 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 😭

@ciampo
Copy link
Contributor

ciampo commented Jul 27, 2022

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.

Yeah, I can totally see that. I would personally try to leverage flexbox and model an elastic behavior

📌 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 😭

Good catch

@jameskoster
Copy link
Contributor

Unless you feel it's a blocker.

I don't think so :)

@mirka mirka force-pushed the update/unit-control-dropdown branch from 1ed6dfc to aa7bf84 Compare August 1, 2022 07:05
@mirka
Copy link
Member Author

mirka commented Aug 1, 2022

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.

@mirka mirka requested a review from ciampo August 1, 2022 07:12
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 🚀

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:

A BorderControl field that has a prefix, number field, and unit dropdown all in a 110px width

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.

@mirka mirka merged commit 56a185a into trunk Aug 2, 2022
@mirka mirka deleted the update/unit-control-dropdown branch August 2, 2022 03:07
@github-actions github-actions bot added this to the Gutenberg 13.9 milestone Aug 2, 2022
@mirka
Copy link
Member Author

mirka commented Aug 2, 2022

Follow-ups moved to #42879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants