AmpersandJS / ampersand-select-view

Select field for ampersand-form-views
MIT License
11 stars 26 forks source link

Error when mixing string and array in options definition #64

Closed e2jk closed 8 years ago

e2jk commented 8 years ago

It's possible to pass simple string options (like options: ['blue', 'orange', 'red']) or pass array (value, label and optional to disable the option, like options: [ ['a', 'Option A'], ['b', 'Option B'], ['c', 'Option C', true] ]).

But if you mix string and array like this options: [ 'Option A', ['b', 'Option B'], ['c', 'Option C', true] ], an error is thrown when selecting one of the options that are not of the same type as the first option in the array: Error: value not in set of provided options

This is due to the fact that in getOptionByValue, check is done based on the type of the first given option:

// find value value in options array
// find option, formatted [['val', 'text'], ...]
if (this.options.length && Array.isArray(this.options[0])) {
  for (var i = this.options.length - 1; i >= 0; i--) {
    if (this.options[i][0] == value) return this.options[i];
  }
}
// find option, formatted ['valAndText', ...] format
if (this.options.length && this.options.indexOf(value) !== -1) return value;
throw new Error('value not in set of provided options');

I think it would make sense to be able to mix both string and array definition, for example if the strings are OK as values but you want to disable just one item.

wraithgar commented 8 years ago

Not sure how I feel about this one honestly. My concern is weighing the potential confusingness of mixing the types of values in a given option against the ease of which you can build an array that does what you want by making both items in the sub-array the same string.

I could be wrong but it seems like it's adding a potential point of confusion for a small amount of syntactic sugar. I'd rather require the options to be more explicit, personally.

Definitely not a hard and firm opinion though. If others really thought this would be better then by all means let's do it.

cdaringe commented 8 years ago

hey guys! ah, i don't like to stifle contributions, but IMHO, i share similar reservations to @wraithgar. from my perspective, 99% of the time <option>s are generated from a homogenous type'd dataset (or group of similar datasets). options source data are then mapd or piped through some generator function, creating a uniform output. conditional options may get tacked on after the fact, which may be manually derived from user interaction or user specific biz logic--in which case could potentially support the PR's intent. however, it's certainly a pattern i personally wouldn't use because it complicates your ability to do other meaningful actions on the .options data.

Definitely not a hard and firm opinion though. If others really thought this would be better then by all means let's do it.

yea, i agree

wraithgar commented 8 years ago

Gonna close this for now. Comments are still open if there is a compelling argument to be had for revisiting this though.

Please don't let this discourage you from contributing @e2jk. Really grateful for the other contributions you've made, and looking forward to your currenlty open PR getting tests and being merged.

e2jk commented 8 years ago

Yep, no problem. I noticed this issue while testing my other change with various input combinations, so reported it here ;)