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: remove pointer-events style from multiselect dropdown #1654

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jorgebv
Copy link
Contributor

@jorgebv jorgebv commented Feb 8, 2023

Resolves #1647
This makes the component more consistent with other components that have the dropdown, and also more consistent with the flagship react library.

@metonym
Copy link
Collaborator

metonym commented Feb 8, 2023

Adding some context here – looking at the Git blame, this hotfix was added in #954 to fix an odd behavior where clicking the icon for a filterable multiselect would double trigger the open behavior.

Agreed that it should be more consistent – is it possible to resolve the previous bug without the pointer-events workaround?

@jorgebv
Copy link
Contributor Author

jorgebv commented Feb 8, 2023

Thanks for the context; I should have thought to look at the blame myself. I will look at the issue again with this context in mind as you suggest.

edit: after some more testing I see that my original tests were not performed correctly; this PR does re-introduce the mentioned bug. will mark the PR as draft while I look into it more.

@jorgebv jorgebv marked this pull request as draft February 8, 2023 18:10
@jorgebv jorgebv marked this pull request as ready for review February 8, 2023 19:34
@jorgebv
Copy link
Contributor Author

jorgebv commented Feb 8, 2023

@metonym I found that the primary cause of #940 was the focus handler of the ListBoxField, which would open the MultiSelect. The pointer events of the ListBoxMenuIcon would then immediately close it.

The flagship react MultiSelect doesn't seem to open on focus, so I removed this behavior from the svelte. Maybe this is a breaking change.

Removing the focus on open caused typing to get lost in the MultiSelect, so I added on:input to the input handler, similar to ComboBox and other components. The on:input handler I have implemented for this MultiSelect is a little basic compared to the ComboBox because I didn't quite see what the purpose of everything the ComboBox is doing on:input. Maybe the on:input handler in MultiSelect is insufficient and needs to be more similar to ComboBox. Feedback welcome.

I also added a little bit of styling for when the input is focused, to get the highlighting of the filterable MultiSelect which was missing before.

This became a little bigger than I wanted it to and a little outside my comfort zone. Maybe some of my approach is undesirable. Please let me know if you see any problems with the overall approach.

@jorgebv
Copy link
Contributor Author

jorgebv commented Mar 27, 2023

@metonym / @theetrain
Sorry to nag on a PR, but do you think this PR has a future? If there's any part of it we want to move forward I'm happy to continue working on it. But if it doesn't seem like the direction of the PR is going to work out, I can close it.

Thanks for volunteering your time together with the community.

@metonym metonym force-pushed the master branch 2 times, most recently from e7485c4 to 417102d Compare November 8, 2023 03:27
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.

Filterable Multiselect Pointer Events
2 participants