-
-
Notifications
You must be signed in to change notification settings - Fork 262
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
base: master
Are you sure you want to change the base?
Conversation
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? |
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. |
@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. |
@metonym / @theetrain Thanks for volunteering your time together with the community. |
e7485c4
to
417102d
Compare
Resolves #1647
This makes the component more consistent with other components that have the dropdown, and also more consistent with the flagship react library.