-
-
Notifications
You must be signed in to change notification settings - Fork 312
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
[Autocomplete] Fix grouped options order #1825
base: 2.x
Are you sure you want to change the base?
[Autocomplete] Fix grouped options order #1825
Conversation
Is the PR planned to be released in the near future? |
Oh i'm so sorry @IndraGunawan .. i remember seeing this PR.... and i forgot to come back on it ! For Autocomplete it seems a very smart & efficient fix: thank you very much for this! Just, before we merge i'd like to be sure this does not add problems/side effets when used with LiveComponent So... did you try within a LiveComponent ? I can help if you want / don't have time, feel free to ask! A good example would be to add groups to the dependant form fields demo of the ux.website (present in the same repository) and see if everything is still working. |
I marked this as reviewed/accepted because the PR in itself is ok ! Let's just wait we check everything is ok with LiveComponent before merging. |
@smnandre Really appreciate your feedback because I myself didn't know if it was a good solution. I was waiting for feedback from you. |
If you don't find time, poke me later in the week i'll make a test or two |
@smnandre Sorry for the delay. Would you like me to update this PR with the changes to the dependent form? |
@zavarock thank you for taking some time to check this! No need to put it in the PR, and i'd rather keep this particular demo the simplest possible (as it's one of the most seen...) But after this PR, it could be a good idea to add a couple of demo / code samples for Autcomplete (only if you want and have time of course) |
@smnandre Sure, no problems!
|
|
How about the status to integrate that issue in the next release, as it is important for the application to have a proper grouped dropdown. As i read it the issue is fixed and working, the demo could be a separate thing to update. |
When can we expect the merge into the next release? |
32c5cdb
to
07b3eeb
Compare
📊 Packages dist files size differenceThanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
|
ce9bd95
to
07b3eeb
Compare
The bug is being caused by TomSelect not preserving the order of the option elements in the select as we select the options in the dropdown. In this case, the MutationObserver callback uses the optgroup element as a parameter to "store" the group (if any) to which the option belongs. However, once the option is selected, it no longer has an optgroup as its parentElement (a problem caused by the aforementioned bug).
As I see it, there is no need to "store" the option's group since, in any case, all elements usually have a unique [value], and even if not, it will still work as expected.
A callback was added for options added through addOption, but the caveat is that it needs the parameter user_created=true to trigger the 'option_add' event.