search

AmpersandJS / ampersand-select-view

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

3.0.0 RC #34

Closed cdaringe closed 9 years ago

cdaringe commented 9 years ago

This PR fixes https://github.com/AmpersandJS/ampersand-select-view/issues/5. The module and tests appeared to be designed intentionally to not fail when setting a bogus value. I.e., if the available options are ['a', 'b', 'c'] and you call setValue('d'), the select would just revert to the 0th index option or unselectedText. This behavior is dangerous. Issue 5 and self experience would indicate the there is a bug in the application code, thus, we now trigger errors when this situation occurs.

This PR additionally:

  1. Enforces that the returned value is always a string. Only if unselectedText option is selected could undefined possibly be appropriate. Would like feedback on this. .value on empty <select>s is generally the empty string, DOM-wise.
  2. Denotes a plan for 4.x release
  3. adds more tests for numeric cases
  4. small tidy ups
nickryall commented 9 years ago

This looks good to me. + 1

cdaringe commented 9 years ago

https://github.com/cdaringe/ampersand-select-view/blob/3.0.0/ampersand-select-view.js#L190 is actually a bug that needs to be addressed. I have permitted short circuit to value '' even if there is no '' option available. Will resolve and address with tests soon.

cdaringe commented 9 years ago

closing for rework