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

[Autocomplete] Fix grouped options order #1825

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

zavarock
Copy link

@zavarock zavarock commented May 3, 2024

Q A
Bug fix? yes
New feature? no
Issues Fix #1616
License MIT

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.

@carsonbot carsonbot added Autocomplete Bug Bug Fix Status: Needs Review Needs to be reviewed labels May 3, 2024
@simonsolutions
Copy link
Contributor

Is the PR planned to be released in the near future?

@IndraGunawan
Copy link
Contributor

@smnandre @kbond any chance to review this?

@smnandre
Copy link
Member

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
(to be transparent: i'm 90% certain this will in fact solve/improve things..... but i'd like to be 100%)

So... did you try within a LiveComponent ?
I'm not talking about coding a test or adding anything more in this PR.. But at least one of us should test manually to check this new behaviour has no impact.

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.

@smnandre
Copy link
Member

⚠️ @kbond

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.

@zavarock
Copy link
Author

zavarock commented Jul 14, 2024

@smnandre Really appreciate your feedback because I myself didn't know if it was a good solution. I was waiting for feedback from you.
Regarding LiveComponent, to be honest, I haven't tested with it yet, but I will later and let you know.

@smnandre
Copy link
Member

If you don't find time, poke me later in the week i'll make a test or two

@zavarock
Copy link
Author

@smnandre Sorry for the delay.
As you suggested, I grouped the options from the dependent form on the UX website and everything seems fine.

ezgif-6-bc9fee1be0

Would you like me to update this PR with the changes to the dependent form?

@smnandre
Copy link
Member

@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 smnandre requested a review from kbond July 20, 2024 18:19
@zavarock
Copy link
Author

@smnandre Sure, no problems!
I just have two questions if you don't mind:

  1. You're talking about adding a new demo on this page, right?
  2. Is it better to create a new PR to avoid mixing things? Or can I just update this one?

@smnandre
Copy link
Member

  1. More probably a dedicated page. I will post a page during the week to explain how to do this properly (short answer, same thing as the LiveComponent demos... but i'll try to isolate more the demos)

  2. I personnaly find better to separate code PRs and website ones.. but that's 100% open to debate :)

@simonsolutions
Copy link
Contributor

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.

@smnandre smnandre requested a review from Kocal September 11, 2024 00:28
@simonsolutions
Copy link
Contributor

When can we expect the merge into the next release?

@smnandre smnandre force-pushed the autocomplete-fix-grouped-options-order branch from 32c5cdb to 07b3eeb Compare November 6, 2024 18:12
Copy link

github-actions bot commented Nov 6, 2024

📊 Packages dist files size difference

Thanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
Please review the changes and make sure they are expected.

FileBefore (Size / Gzip)After (Size / Gzip)
Autocomplete
controller.js 15.04 kB / 3.62 kB 16.47 kB+9% 📈 / 3.93 kB+9% 📈

@smnandre smnandre force-pushed the autocomplete-fix-grouped-options-order branch from ce9bd95 to 07b3eeb Compare November 6, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display Problem UX Autocomplete / TomSelect
5 participants