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

Enable search for Color By #1456

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Conversation

victorlin
Copy link
Member

Description of proposed changes

This makes the Color By dropdown searchable as the list gets longer.

Enhancement proposed by @trvrb, implementation from @joverlee521

There are more non-searchable Selects, would it be appropriate to enable elsewhere? https://github.com/nextstrain/auspice/search?q=searchable%3D%7Bfalse%7D

@nextstrain-bot nextstrain-bot temporarily deployed to auspice-searchable-colo-igpgfs February 10, 2022 21:58 Inactive
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the change @victorlin! Works as expected in the testing app.

I had wondered (very) briefly if we wanted to toggle the search function based on the number of options in the dropdown, but that seems like it would be overcomplicating things for no good reason.

There are more non-searchable Selects, would it be appropriate to enable elsewhere?

A majority of the other drop downs only have a handful of options that don't really warrant searching, but I also don't see any harm in enabling the search function. @jameshadfield would have more context for why they have search disabled.

@trvrb
Copy link
Member

trvrb commented Feb 11, 2022

On desktop, I think it's a clear win always having search. However, tapping on searchable dropdowns on mobile brings up a keyboard. This is annoying if there's just a couple options.

Basically, I'd think to enable search when there's more than 5 options. This is when you need to scroll.

@victorlin victorlin merged commit bff18e0 into nextstrain:master Feb 11, 2022
@victorlin victorlin deleted the searchable-colorby branch February 11, 2022 17:14
@victorlin
Copy link
Member Author

Merged but we can continue discussion here.


Another option is to conditionally enable search if desktop or (mobile and >5 options). Mobile can be detected:

isMobile={this.props.browserDimensions.width < controlsHiddenWidth}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants