support <optgroup> elements

[e2jk]

Fixes #61

Because the options parameter is not only used to render the <select> but also to perform other tasks such as in updateSelectedOption or getOptionByValue, I've created a new groupOptions parameter which will be used to create the various <optgroup> elements at rendering time, and from which the options parameter will be created that contains only the values that appear in <options> elements. That way, the rest of the features work just the same as if just options was passed.

I have not created any tests, as I'm not sure how to run them. Help would be appreciated here.

You can try different values of groupOptions:

simple (just text options): groupOptions: [ {groupname: "Options 1", options: ["Option 1.1", "Option 1.2"] }, {groupname: "Options 2", options: ["Option 2.1", "Option 2.2"] } ]

or more complex (with arrays as options to define different values and disable some elements):

groupOptions: [ {
                  groupname: "Options 1",
                  options: [ ['1', 'Option 1'], ['2', 'Option 2'], ['3', 'Option 3', true] ]
                },
                {
                  groupname: "Options 2",
                  options: [ ['a', 'Option A'], ['b', 'Option B'], ['c', 'Option C', true] ]
                }
              ]

[e2jk]

Not sure why tests are failing. I see the following mention

Error:

Zuul tried to run tests in saucelabs, however no saucelabs credentials were provided.

See the zuul wiki (https://github.com/defunctzombie/zuul/wiki/Cloud-testing) for info on how to setup cloud testing.

Again, help would be appreciated around creating test cases and figuring out how to pass the CI build. Thanks!

[wraithgar]

The CI build is a currently ongoing issue we're trying to solve. Because this PR comes from a fork, the encrypted environment variables for saucelabs aren't provided during tests.

When you npm install locally a git precommit hook is set up that runs tests/lints/validates before commit. Those local tests run against phantomjs. You should see something to the effect that tests are being run when you commit.

You can also run npm test locally to manually run the tests.

[cdaringe]

Looks good! only two comments, and of course getting some tests generated! if gar's note on how to run them was unclear, write back. would love to show you the ropes otherwise.

[e2jk]

OK, creating the tests was actually easy once I had sudo npm install'd away. I've duplicated all the tests that are run for the options parameter, for strings as well as for arrays, all all goes fine.

There is one more thing that needs to be addressed before this can be merged (pending your review, of course): in 8502da9 I've commented out the console.warn that I had set up to warn the developers that if they define both options and groupOptions, the former will be ignored. Lint (correctly) doesn't like console, and I don't want to throw a new Error but still want to inform the developers to prevent hours of lost time if someone hits this. What would be the best way to do so? Or should I just throw an Error and be done with it?

Let me know if I should squash/rebase my commits into a single one, depending on the policy/custom of the project.

Thanks for reviewing!

[wraithgar]

Oh definitely throw the error. No squash or rebase needed.

Not sure about the changelog position. This would ship as a minor release, and I think we're only maybe documenting breaking changes in there? @cdaringe will probably have a better grasp on that.

[cdaringe]

yep yep. it will be a minor. feel free to mark it as 6.1.0 in the log, as that's what it will roll in as. thanks again for the PR. appreciate the tests being added! the tests in this package are a little intense (e.g. checking the content of each node). we may reduce them down at some point just to make the massive file a little more digestible, without sacrificing quality of course ;).

[e2jk]

Throwing error, and updated changelog. I've added a test to make sure the error is thrown, and I'm using t.throws. But it looks like the rest of the test app uses a try/catch: let me know what you want to do with it.

[wraithgar]

throws is just fine, not really concerned about consistency in the tests so much as coverage.

[cdaringe]

tests pass locally for me. +1. i'll probably do a little general README tidy prepublish

[wraithgar]

Yeah +1 go for it, :shipit:

[cdaringe]

patched some spacing issues and missing < > escaped chars. published as 6.1.0.

[e2jk]

Great, thanks!