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

Added properties for using dropdown in search #385

Closed
wants to merge 2 commits into from

Conversation

sidharthramesh
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Mar 22, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shoelace/shoelace/H6rP5BKpFBt9ZfLvbwWyhpLkHmnX
✅ Preview: https://shoelace-git-fork-sidharthramesh-next-shoelace.vercel.app

@claviska
Copy link
Member

What is the intent here? Is it to simply disable typeToSelect functionality? It's not clear what the focusKeys prop is being used for.

@sidharthramesh
Copy link
Contributor Author

@claviska the intent is to use sl-dropdown with an sl-input in the trigger slot. This provides good functionality as a search drop-down/combobox.

The typeToSelect Boolean is to disable/enable that function. The focusKeys props is so that spacebar or enter can be manually disabled if needed. In the case of a search component, spacebar must be disabled so that a space character can be entered in the input.

Although the methods can be overridden easily in the context of a search component, I thought these two should be part of the API for easier customisation.

@claviska
Copy link
Member

The focusKeys props is so that spacebar or enter can be manually disabled if needed. In the case of a search component, spacebar must be disabled so that a space character can be entered in the input.

I'm only seeing focusKeys defined as a prop. I'm not seeing it used anywhere.

In the case of a search component, spacebar must be disabled so that a space character can be entered in the input.

This may cause a11y issues so we need to be careful here.

Is this more or less what you're trying to achieve?

CleanShot 2021-03-22 at 17 37 24@2x

If so, it probably needs to be incorporated into <sl-menu> since that's where most of the keyboard logic lives. We also need to think about how arrow keys and tabbing should behave (right now arrows only traverse menu items and shift + tabbing back into the input won't work because of that).

To be honest, this is probably a separate component that utilizes <sl-dropdown> and replaces <sl-menu> with custom behavior. I don't think it's wise to hack this into place with composition because it won't be fully accessible nor easy to maintain as a pattern.

@sidharthramesh
Copy link
Contributor Author

Sorry, my bad. I forgot to cherry-pick a commit. My use case is more like this:

image

@claviska
Copy link
Member

claviska commented Apr 8, 2021

I appreciate the PR. At this point, I don't feel comfortable exposing these options. Let's revisit this as part of #127 after the 2.0 stable release. 😄

@claviska claviska closed this Apr 8, 2021
@staabm
Copy link
Contributor

staabm commented Apr 15, 2021

If so, it probably needs to be incorporated into <sl-menu> since that's where most of the keyboard logic lives. We also need to think about how arrow keys and tabbing should behave (right now arrows only traverse menu items and shift + tabbing back into the input won't work because of that).

To be honest, this is probably a separate component that utilizes <sl-dropdown> and replaces <sl-menu> with custom behavior. I don't think it's wise to hack this into place with composition because it won't be fully accessible nor easy to maintain as a pattern.

we have a similar need and will try to build a searchable select like you proposed (to get rid of chosen.js components we have right now)

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.

3 participants