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

Fix/select control multiple #43213

Closed

Conversation

mediaformat
Copy link
Contributor

@mediaformat mediaformat commented Aug 14, 2022

What?

This fix removes the double-arrow from multiple select.

Why?

SelectControl component with multiple enabled currently displays the double arrow associated with the pop-over.

Original issue #27166

This PR builds on the Original PR
#28817

How?

Conditionally render double arrow if multiple isn’t enabled
Also add a storybook option to display multiple

Testing Instructions

Visible on desktop view of storybook
https://wordpress.github.io/gutenberg/?path=/story/components-selectcontrol--default&args=multiple:true

Screenshots or screencast

@mediaformat mediaformat requested a review from ajitbohra as a code owner August 14, 2022 06:20
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Aug 14, 2022
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @mediaformat! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@mediaformat
Copy link
Contributor Author

I still need to test this with react-native

@mediaformat
Copy link
Contributor Author

mediaformat commented Aug 15, 2022

New SelectControl multiple test on Storybook https://mediaformat.github.io/gutenberg/?path=/story/components-selectcontrol--select-multiple

Current style fixes did not account for mobile, will adjust

@mirka mirka added the [Package] Components /packages/components label Oct 13, 2022
@mirka mirka requested review from mirka and ciampo October 13, 2022 08:37
@mirka
Copy link
Member

mirka commented Dec 20, 2022

I still need to test this with react-native

No need for this, the component for react-native is a wholly different implementation 👍

@mediaformat
Copy link
Contributor Author

@mirka the component, renders differently depending on desktop or mobile, i imagine it's based on device touch. Is there a WP way to detect this?

@mirka
Copy link
Member

mirka commented Dec 21, 2022

@mirka the component, renders differently depending on desktop or mobile, i imagine it's based on device touch. Is there a WP way to detect this?

Do you mean the responsive styles in the Sass file, or the special appearance on iOS Safari (not sure what happens on Android)?

Sass file — There seem to be a lot of redundant styles in there that are already being overridden by the Emotion file. Now might be a good time to clean that up if they are clashing in any way with what we're trying to do here.

iOS — I think the best way would be to just stick with height: auto in all size variants in the multiple case (regardless of desktop/mobile), instead of hardcoding values.

Demos

iOS

RPReplay_Final1671641827.mov

Desktop

Select options fully expanded

All options expanded by default. Maybe we could add a reasonable size attribute to the <select> so it doesn't get too long. Technically we could even add an htmlSize prop or something on the component that forwards to the <select> as a standard HTML size attribute so a consumer can set their own heights, but I'd rather not. We should keep the multiple-related code as simple as possible because as a components library we don't really recommend this usage and it isn't worth the maintenance cost.

@ciampo
Copy link
Contributor

ciampo commented Jan 2, 2023

Hey @mediaformat , let us know if you need any extra help on moving this one forward!

@mirka
Copy link
Member

mirka commented Feb 2, 2023

This is a long-standing issue and I'd like to resolve it sooner rather than later. @mediaformat are you still available to work on this? If not I'd be happy to take over 🙂

@ciampo
Copy link
Contributor

ciampo commented Feb 4, 2023

If we aim at releasing this fix with the next WordPress release, we should resume work on this issue fairly soon. I guess we can decide a date by which we can take over and ship the fix :)

@mirka
Copy link
Member

mirka commented Feb 8, 2023

I will be taking over to get this into WP 6.2. Thanks for working on this, @mediaformat!

👉 #47893

@mirka mirka closed this Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants