Optgroup feature for select and multipleSelect Widgets

[tolleiv]

Hi,

thanks for this great form framework. I missed an option to have "optgroups" within the select widgets and therefore added an option to have them.

For now I've kept the functions to render the groups and options separate per widget, but that could be refactored later on to have a single rendering function for both.

I've also added basic tests, which also show the configuration syntax.

Would be great to get your feedback.
Cheers,
Tolleiv

[ljharb]

Instead of:

choices: {
  foo: 'bar',
  group1: {
    label: 'Hamster Dance',
    choices: {
      1: 'one',
      2: 'two'
    }
  }
}

as a data format to create an optgroup, what would you think about this format:

choices: {
  foo: 'bar',
  'Hamster Dance': {
    1: 'group1:one',
    2: 'group1:two'
  }
}

That might be more compact, and also would remove the implicit value munging that happens with "group1", and might also make the implementation a bit cleaner?

[tolleiv]

Your suggestion looks like my first attempt (which I tried locally) - but that made it harder to have just one function to render the options and made the code quite ugly. I also found it nice to have an option to specify the label explizit.

Regarding the value munging - you're right, I also don't like it, but it would still be needed in the case were the options are passed as an array:

choices: {
  foo: 'bar',
  'Hamster Dance': [
    'group1:one',
    'group1:two'
  ]
}

But I'm fine with your suggestion - that would mean that arrays can't be used in these cases.
I can try to come up with a changed pull request tomorrow. Just let me know what you think about the "array" issue?

Cheers

[ljharb]

I don't think the options should ever be passed as an array - imo the value and text should always be explicitly specified, which requires an object (since string parsing is an antipattern).

[tolleiv]

Ok ... then I'll go with your suggestion. But the new patch has to wait until tomorrow

[tolleiv]

I've adjusted the code and extended the tests a bit.

[ljharb]

Please use is.object(choices[k]) here - typeof null is "object", and please always avoid == in any circumstances.

[ljharb]

Other than these two comments, this looks great. Thanks!

[ljharb]

@tolleiv Not sure how this fell through the cracks - can you freshly rebase it on top of latest master if you're still interested in landing it?