gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

SelectControl: Add "minimal" variant

Open mirka opened this issue 1 year ago • 14 comments

Closes #57720

What?

Adds a "minimal" variant to SelectControl.

Why?

Some use cases in Data Views are calling for a borderless variant.

Testing Instructions

See the "Borderless" story for SelectControl.

Testing Instructions for Keyboard

It should still show a focus indicator when focused.

Screenshots or screencast

Ideally, the SelectControl width hugs the currently selected option. There is a CSS field-sizing property in the pipeline that does exactly what we want, and it's shipping in Chrome but we're still waiting on Safari and Firefox. (They are both planning to implement it though.)

To implement this for all browsers now, we need to add a bit of JS complexity (inject a hidden select with only the selected option, get its computed width, then explicitly set the width of the actual select). It would be great if we could avoid adding this complexity now, and treat this width-hugging as a progressive enhancement until field-sizing arrives in Safari and Firefox.

As a fallback, the width will adjust to the longest option in the set. (Unlike the "default" variant, where the width takes up all the available space.)

Chrome Firefox Safari
Borderless variant in Chrome Borderless variant in Firefox Borderless variant in Safari

mirka avatar Jul 08 '24 17:07 mirka

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Co-authored-by: jasmussen <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions[bot] avatar Jul 09 '24 16:07 github-actions[bot]

Also, I wonder if we should pick a variant name that is less literal and tied to the border, since the border removal is not the only change applied in this variant (and for better flexibility in the future). Maybe something like lean ?

I'm not too worried about "borderless" for the moment, but "minimal" could be another possibility?

mirka avatar Jul 10 '24 12:07 mirka

+1 for the more semantic name. 'Minimal' seems good.

This implementation will work well for the filter operator UI, this one:

Screenshot 2024-07-10 at 18 05 59

I'm curious to gather more thoughts on the page selection design though (cc @mtias @jasmussen), which this implementation will not serve 100% as things stand:

Screenshot 2024-07-10 at 18 06 02

Two points:

  • The typography is different, is there a way to avoid overrides, or would it be acceptable in this case?
  • It doesn't seem ideal for each option to be so verbose, imagine a screen reader announcing options "Page 1 of 3, Page 2 of 3, Page 3 of 3..." and so on.

It might be better to do something more minimal using the inline label option:

Screenshot 2024-07-10 at 17 36 40

Speaking of... the inline label option causes the select to fill the container, is that fixable? Screenshot 2024-07-10 at 17 39 00

jameskoster avatar Jul 10 '24 16:07 jameskoster

I'm curious to gather more thoughts on the page selection design though

The design is solid enough, and I would specifically think that a minimal select could be useful for cases like that, I'd think it a primary use case. Can variant inherit text styles, instead of come with its own?

jasmussen avatar Jul 11 '24 07:07 jasmussen

Can variant inherit text styles, instead of come with its own

That's an interesting idea.

The design is solid enough

Which one? :)

As it stands, this design would result in extremely verbose options:

Screenshot 2024-07-10 at 18 06 02

The alternative would be to do something like this so the options can simply be "1, 2, 3, 4, 5...":

Screenshot 2024-07-10 at 17 36 40

jameskoster avatar Jul 11 '24 13:07 jameskoster

The alternative would be to do something like this so the options can simply be "1, 2, 3, 4, 5...":

From previous conversations we should be careful in removing the "of [total pages]" text, especially given that we're already removing visual prominence from the control.

ciampo avatar Jul 11 '24 13:07 ciampo

Thanks for the reminder @ciampo. If the verbose options aren't considered a big problem, then the "Page x of y" design is the way to go.

Did you have any thoughts about the type styles?

jameskoster avatar Jul 12 '24 08:07 jameskoster

If the verbose options aren't considered a big problem

I don't think so. From what I remember, verbosity in this scenario was seen as a plus, as it gave a lot of context (especially to users that don't rely on sight).

the inline label option causes the select to fill the container, is that fixable?

Looking at Storybook, think that in general SelectControl always fills the width of its container.

Not sure if it's a good idea to change the behaviour, but in the worst case scenario you could always wrap SelectControl in another HTML element (or set something like width: fit-content on it)

The typography is different, is there a way to avoid overrides, or would it be acceptable in this case? ... Can variant inherit text styles, instead of come with its own? ... Did you have any thoughts about the type styles?

Are you proposing that the select "trigger" (ie. the value shown by the dropdown when collapsed) uses the same styles of the control labels? Or that it inherits whatever text styles would come from the CSS cascade?

ciampo avatar Jul 12 '24 14:07 ciampo

Speaking of... the inline label option causes the select to fill the container, is that fixable?

@jameskoster This shouldn't be the case 🤔 It should hug the current option text (Chrome) or adapt to the longest option (Firefox, Safari). Can you still reproduce?

Did you have any thoughts about the type styles?

From a technical standpoint (don't know about the design systems standpoint), I am fine with an override in this case. I'd say it's safe.

<SelectControl className="my-select-control" />
.my-select-control select {
  text-transform: uppercase;
}

mirka avatar Jul 12 '24 14:07 mirka

Can you still reproduce?

Yup, here's a demo:

https://github.com/user-attachments/assets/341cf0ca-6160-4e9c-9e04-3e919a245c6f

jameskoster avatar Jul 12 '24 16:07 jameskoster

Yup, here's a demo:

Ah ok, good catch. I fixed it in labelPosition="edge" (fbcbbaeff4d3355190c33e5d8d6c93287a19d051), but I'd say the behavior is correct in labelPosition="side". I also noticed that labelPosition="edge" cannot be utilized in the default variant SelectControl (#63586), but that's a separate issue.

mirka avatar Jul 15 '24 21:07 mirka

Sidenote, noticed this:

const SelectControlChevronDown = () => {
	return (
		<InputControlSuffixWrapperWithClickThrough>
			<DownArrowWrapper>
				<Icon icon={ chevronDown } size={ chevronIconSize } />
			</DownArrowWrapper>
		</InputControlSuffixWrapperWithClickThrough>
	);
};

That appears to use the full-size chevronDown icon. It should use the chevronDownSmall unscaled, no? We might need to add some negative margins left and right of the icon itself, so the chevron is close to the text despite the 24px footprint. Not a blocker for this PR, just noticed.

jasmussen avatar Jul 16 '24 07:07 jasmussen

That appears to use the full-size chevronDown icon. It should use the chevronDownSmall unscaled, no?

Ok, I'll make a note to follow up on this 👍

mirka avatar Jul 16 '24 10:07 mirka

Does anyone have any blockers to merging this? FWIW it's as early as we can be in the 6.7 release cycle so we'll have a good amount of time to iterate on the details.

mirka avatar Jul 16 '24 10:07 mirka

Looking good for me, we can merge if we have the green light from the design team 🚀

There is a CSS field-sizing property in the pipeline that does exactly what we want, and it's shipping in Chrome but we're still waiting on Safari and Firefox. (They are both planning to implement it though.)

Should we add a TODO in the code and/or open an issue for using field-sizing when available to all browsers?

ciampo avatar Jul 16 '24 15:07 ciampo

There is a CSS field-sizing property in the pipeline that does exactly what we want, and it's shipping in Chrome but we're still waiting on Safari and Firefox. (They are both planning to implement it though.)

Should we add a TODO in the code and/or open an issue for using field-sizing when available to all browsers?

This PR already adds the styles that will work in browsers with field-sizing implemented (Chrome), and there are no extra styles to clean up when all browsers support it. So I think we're good.

mirka avatar Jul 16 '24 16:07 mirka

🤦 I looked for field-sizing in the code, and didn't check for fieldSizing

ciampo avatar Jul 17 '24 07:07 ciampo

Seems good to me!

jameskoster avatar Jul 17 '24 10:07 jameskoster

Removing the Needs Dev Note label because we don't necessarily want to advertise this variant when it's still quite niche in Gutenberg itself.

mirka avatar Oct 10 '24 13:10 mirka

Hi @mirka, I have identified that fieldSizing: 'content' does not add right padding on Windows. This can be reproduced by changing the variant in Storybook on a Windows machine using the Chrome (or Edge) browser. CleanShot 2025-10-15 at 18 38 04@2x

Do you think we should remove the variantStyles for the Select so we can keep the borderless variant but avoid the issue on Windows? That would mimic the behavior in Chrome, Safari, and Firefox (as Safari and Firefox do not currently support field-sizing).

epeicher avatar Oct 15 '25 16:10 epeicher

Hmm, good one. Can you post an issue so we can keep track of it? Also, does it still occur if the longest option value is not disabled? I'm wondering why it hasn't been reported until now, given how big the affected user segment is. One reason might be that the minimal variant is only used in limited places, and none of the first-party usage in Gutenberg is affected. From a quick check on BrowserStack, I couldn't find any problem instances within Gutenberg (e.g. DataViews, ColorPicker). Maybe because the options are all shorter than a given threshold, or maybe because none of the options are disabled.

We are planning to introduce a new Select component to the repo within the next few weeks, which is fully custom rendered and can avoid this cross-browser issue. So my preferred course of action would be to wait for that new component to land, and leave this one alone, given the low severity.

mirka avatar Oct 15 '25 19:10 mirka

I was able to confirm that this only happens when the option is longer than the currently selected value. Nothing to do with disabled attributes. Probably hasn't manifested because the option lengths are pretty much similar in all of our first-party use cases.

mirka avatar Oct 16 '25 14:10 mirka

Thanks for the input and the additional info @mirka!

Can you post an issue so we can keep track of it?

Yep -> https://github.com/WordPress/gutenberg/issues/72424

I'm wondering why it hasn't been reported until now, given how big the affected user segment is. One reason might be that the minimal variant is only used in limited places, and none of the first-party usage in Gutenberg is affected. From a quick check on BrowserStack, I couldn't find any problem instances within Gutenberg (e.g. DataViews, ColorPicker). Maybe because the options are all shorter than a given threshold, or maybe because none of the options are disabled.

I think is because is not used that much as you suggest, for example in Calypso code is just used once. As you mention in your other comment this is not related to the disabled attribute, but to the length of the option being larger than the default value. I have reproduced with this simple codepen that fails if you open it on Chrome on Windows - link here

We are planning to introduce a new Select component to the repo within the next few weeks, which is fully custom rendered and can avoid this cross-browser issue. So my preferred course of action would be to wait for that new component to land, and leave this one alone, given the low severity.

That makes sense, I have created the issue, but it can be closed if there is a new Select component that fixes this.

epeicher avatar Oct 17 '25 09:10 epeicher