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.

Sign up for GitHub

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

Jump to bottom

optgroup does not work when some options are nested and some are not #615

Open
EvanLovely opened this issue Jul 16, 2019 · 4 comments · May be fixed by #1110
Open

optgroup does not work when some options are nested and some are not #615

EvanLovely opened this issue Jul 16, 2019 · 4 comments · May be fixed by #1110

Comments

@EvanLovely
Copy link

EvanLovely commented Jul 16, 2019 •
edited

The library used works if every <option> is in a <optgroup> but has a bug when some are nested and some are not.

This will work:

  <select>
    <optgroup label="UK">
      <option value="London">London</option>
      <option value="Manchester">Manchester</option>
    </optgroup>
    <optgroup label="FR">
      <option value="Paris">Paris</option>
      <option value="Marseille">Marseille</option>
    </optgroup>
  </select>

This will not:

  <select>
    <option value="NYC">New York</option>
    <optgroup label="UK">
      <option value="London">London</option>
      <option value="Manchester">Manchester</option>
    </optgroup>
    <optgroup label="FR">
      <option value="Paris">Paris</option>
      <option value="Marseille">Marseille</option>
    </optgroup>
  </select>

Related: #253
@Geolim4
Copy link

Geolim4 commented Aug 31, 2022

Hello @mtriff, can you take a look at this bug eventually please ? 😭

voidus added a commit to voidus/Choices that referenced this issue Mar 31, 2023
@voidus
Add failing cypress test for Choices-js#615
f84c738
@voidus
Copy link

voidus commented Mar 31, 2023

I have started to look into this, but didn't get very far yet. I do have a failing cypress test though, if anyone wants to support: https://github.com/voidus/Choices/tree/fix-mixed-optgroups

Next step: Find the relevant place in the code

voidus added a commit to voidus/Choices that referenced this issue Mar 31, 2023
@voidus
Some work towards possibly fixing Choices-js#615
423195d
@voidus
Copy link

voidus commented Mar 31, 2023

Okay I've oriented myself a bit in the code and found an approach.

Right now, afaict, the parts that process the options deal with either Choice objects or the html options, and my approach is to convert the html elements to Choice objects first. This should enable us to simplify the processing code.

TBH, I was expecting the change on the branch to already have an effect, but it doesn't look like it.
If anybody wants to continue, please don't hesitate to grab the code!

@mtriff would you be generally open to merge something like this? Otherwise we should probably discuss potential approaches. (Tagging you because you seem to be maintaining the repo)

voidus added a commit to voidus/Choices that referenced this issue Apr 4, 2023
@voidus
Some work towards possibly fixing Choices-js#615
05a7ad9
voidus added a commit to voidus/Choices that referenced this issue Apr 4, 2023
@voidus
Render options without a group even if groups are present.
d644d39 
Closes Choices-js#615.

This pushes the conversion of OPTION/OPTGROUP elements to Choice objects
into the WrappedSelect class and unifies the code paths a little between
groups-present and groups-not-present.

Some work towards possibly fixing Choices-js#615
voidus added a commit to voidus/Choices that referenced this issue Apr 5, 2023
@voidus
Render options without a group even if groups are present.
9423065 
Closes Choices-js#615.

This pushes the conversion of OPTION/OPTGROUP elements to Choice objects
into the WrappedSelect class and unifies the code paths a little between
groups-present and groups-not-present.

Some work towards possibly fixing Choices-js#615
@voidus voidus linked a pull request Apr 5, 2023 that will close this issue
Open
10 tasks
@voidus
Copy link

voidus commented Apr 5, 2023

Yeah folks I think I got it. 🕺
Let's see if this is taking the code in a direction that @mtriff is happy with :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix mixed optgroups voidus/Choices
3 participants
@voidus @EvanLovely @Geolim4