Skip to content

Duplicated options in different optgroups doesn't render correctly #1128

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

Closed

Conversation

zeitiger
Copy link
Contributor

@zeitiger zeitiger commented Aug 9, 2016

Duplicated options (options with the same value) will be render only once within last occurrence

This pull request includes a test case to reproduce the problem and the bugfix for that. I'm open for discussion to make this bugfix better :-)

Have a nice day 😸

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Changes Unknown when pulling a2143c7 on zeitiger:brokenOptgroupDuplication into * on selectize:master*.

@joallard
Copy link
Member

joallard commented Aug 9, 2016

Thanks for the PR. If I'm not mistaken that should be part of the wider improvement that duplicate values should be allowed regardless of optgroups (see #129). Sounds like those two should probably go hand in hand.

@zeitiger
Copy link
Contributor Author

zeitiger commented Aug 10, 2016

I believe that is a simple bug, because the complete preprocessing of optgroup and option within init_select with addGroup and addOption had already in mind how I expect that a select should work

// if the option already exists, it's probably been
// duplicated in another optgroup. in this case, push
// the current group to the "optgroup" property on the
// existing option so that it's rendered in both places.

and also another test case give me the hint

it('should allow for values optgroups with duplicated options', function() {

but that fall short, because the DOM works different as expect for the original author within rendering step.

The issue want a change within the data processing step before the rendering. If you want that that you have to change the internal data structure, that will be a much bigger refactoring.

IMHO my pull request is as render bug fix independent of data processing change request

@zeitiger
Copy link
Contributor Author

zeitiger commented Aug 11, 2016

This already existing example (optgroups.html) is IMHO broken at the moment

            <div class="demo">
                <h2>Optgroups (repeated options)</h2>
                <div class="control-group">
                    <label for="select-repeated-options">Options:</label>
                    <select id="select-repeated-options" class="demo-default" multiple>
                        <option value="">Select...</option>
                        <optgroup label="Group 1">
                            <option value="a">Item A</option>
                            <option value="b">Item B</option>
                        </optgroup>
                        <optgroup label="Group 2">
                            <option value="a">Item A</option>
                            <option value="b">Item B</option>
                        </optgroup>
                    </select>
                </div>
                <script>
                $('#select-repeated-options').selectize({
                    sortField: 'text'
                });
                </script>
            </div>

current version render this version
bildschirmfoto 2016-08-11 um 13 07 30

and my change will make it render this way
bildschirmfoto 2016-08-11 um 13 08 16

@zeitiger zeitiger force-pushed the brokenOptgroupDuplication branch 3 times, most recently from 191c94a to 81bbdc4 Compare August 11, 2016 13:33
@zeitiger
Copy link
Contributor Author

I made an entry in the changelog and rebase this pull request to make it easier to merge :-D

@joallard
Copy link
Member

I will check it out, thanks @zeitiger

@joallard joallard added this to the 0.13.0 milestone Aug 11, 2016
@zeitiger zeitiger force-pushed the brokenOptgroupDuplication branch from 81bbdc4 to 59a3e4a Compare August 18, 2016 08:46
@yaquawa
Copy link

yaquawa commented Jun 18, 2018

Same problem here, still not merged yet!

@joallard
Copy link
Member

If I could get a rebase on master, I'd be inclined to merge.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2021

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants