-
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
SelectControl: Add "minimal" variant #63265
Conversation
9ede006
to
3f96f97
Compare
isBorderless
prop
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. |
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, although I'll let @jameskoster experiment with this component in Data Views first before approving to make sure this new variant fulfills the end goal there.
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 good at naming 🙈
I'm not too worried about "borderless" for the moment, but "minimal" could be another possibility? |
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.
Looking good 👍
FWIW I like minimal
a lot, it provides enough flexibility that doesn't bind us to specific style properties like border
.
+1 for the more semantic name. 'Minimal' seems good. This implementation will work well for the filter operator UI, this one: 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: Two points:
It might be better to do something more minimal using the inline label option: Speaking of... the inline label option causes the select to fill the container, is that fixable? |
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? |
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. |
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? |
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).
Looking at Storybook, think that in general Not sure if it's a good idea to change the behaviour, but in the worst case scenario you could always wrap
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? |
@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?
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;
} |
Yup, here's a demo: wide-select.mp4 |
Sidenote, noticed this:
That appears to use the full-size |
Ok, I'll make a note to follow up on this 👍 |
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. |
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.
Tests well in Storybook, code looks good, no blockers from my side. 👍
Looking good for me, we can merge if we have the green light from the design team 🚀
Should we add a TODO in the code and/or open an issue for using |
This PR already adds the styles that will work in browsers with |
🤦 I looked for |
Seems good to me! |
* SelectControl: Add `isBorderless` prop * Switch to `variant` * Add `field-sizing` styles * Add InputBase styles * Add changelog * Fixup * Fixup changelog * Rename "borderless" to "minimal" * Fix in `labelPosition="edge"` Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
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. |
Closes #57720
What?
Adds a
"minimal"
variant toSelectControl
.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.)