Conversation
Signed-off-by: Tom Kremer <kremer.tom@gmail.com>
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
This currently isn't passing the linter, and would need tests.
In general, I like this addition!
lib/widgets.js
Outdated
| var renderData = isNested ? | ||
| { isNested: true, label: choice[0], choices: choice[1] } : | ||
| { isNested: false, value: choice[0], label: choice[1] }; | ||
| if (choice.length == 3) { |
There was a problem hiding this comment.
i'm not sure we need this branching - we can just set disabled to false unless a third truthy value is provided.
| var renderData = isNested ? | ||
| { isNested: true, label: choice[0], choices: choice[1] } : | ||
| { isNested: false, value: choice[0], label: choice[1] }; | ||
| { isNested: true, label: choice[0], choices: choice[1], disabled: choice[2], default: choice[3] } : |
There was a problem hiding this comment.
what is "default"? let's keep this PR just to adding "disabled".
|
i gotta comment even though I’m not a maintainer but it’s my code you’re changing :). i like the idea, but apart from the fact, that this change only provides an array syntax for specifying disabled options, i always feel like options that use designated array indexes are hard to remember and force developers to look at the docs a lot because they forgot which option is which array-index. the wish to omit the disable or default flag (this pr introduces both as of now) and the need to have special values to skip an array index is another problem i see with this implementation. why not keep the current format as it is and add defaults and disabled values as individual field options? iirc values are unique for each field and you could just build object where choice values map to default values/ disabled flags. that’d be backwards compatible, easy to remember and you probably would’t have to touch any code beside the choice renderer. btw. how do you see choice defaults working out? i might be mistaken but I don’t see the point because you already have full control over the values as the user simply selects given choices. |
|
I haven't truly yet understood this PR, but I agree that anything that requires remembering or matching array indices is just not a good API. |
3 participants
Set the third value in choices to TRUE to disable an item on the list.