-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow to disable options with selectable function #921
Conversation
Works well, thanks for your work! A few issues:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think once doits#1 is merged this one is good to go as well. Thanks so much for the great work!!
Thanks for the PR @tobyzerner and fixing those remaining issues in doits#1 - I'll respond here about the business logic so we have everything in one place about it. I agree with everything but I'm not sure if disabled options should be removed when filtering looking from a business logic stand point. Because actually it might be good to show the user the the disabled option so the user knows there is that option but it is simply not selectable. Maybe because it is temporary disabled or because of some precondition. Otherwise he might think this option does not exist at all. Of course this depends on what you use disabled options for. Maybe you have something in mind where showing disabled options when filtering makes no sense. Can you maybe give an example use case? |
True, that's a very good point. My use case for disabled options is actually to simulate optgroups which is why I don't want them to appear in filtering. So it's probably not a good idea to include that logic by default – I've removed it and rebased my PR. For my particular use-case, I realised I can use the |
Fix some selectable functionality
Alright, I merged it, should be ready to go now. |
@sagalbot feel free to merge and release this too, if everything is well here :-) |
Implemented as per #309 (comment)
ToDo:
To test this in your project, you can use my custom branch which incoperates this PR and #914 and has built packages.
Disclaimer: Not for production use, only provided for testing purposes. Might be bugged and eat your data.
In your
package.json
:Closes #104