search

dequelabs / combobo

Accessible combobox
https://dequelabs.github.io/combobo/demo/
MIT License
49 stars 13 forks source link

Deselect current option when selected #16

Closed lawshe closed 6 years ago

lawshe commented 6 years ago

Thanks so much for building this!

I ran into a problem where I couldn't deselect an option. Seems like it was due to the selected class on the current option being removed before the conditional to check if deselection should be emitted.

I was able to successfully deselect an option after removing lines 308-311 in index.js.

schne324 commented 6 years ago

Thanks for the using Combobo and contributing! There are unit tests that are failing with this change (try running npm test), but I think I know what you're getting at here. The behavior you are seeing (not being able to deselect after a selection has been made) is intended but I would agree - it does not make sense for every implementation. The first application I used Combobo in required this functionality but I am now realizing that we should have added configuration option for this behavior.

I think this problem should be solved with options.allowEmpty (or something like that - naming things are hard!). That way, when a Combobo is instantiated with a truthy allowEmptyoption, the functionality you've added will take place. Otherwise, when falsey it works as it does today.

new Combobo({  allowEmpty: false });

Does that sound like something you'd be interested in tweaking this pull request to do? If not its no big deal, I can just raise an issue and get to it when I have time, otherwise I would really appreciate it!

schne324 commented 6 years ago

Closing this PR because I never received a response. This feature is added in #20