search

AmpersandJS / ampersand-select-view

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

Added support for number type attributes #68

Open dw2 opened 8 years ago

dw2 commented 8 years ago

This change will allow model attributes with type 'number' to work with the select view. As an added bonus, this will also allow for mixed option sets (i.e. [[0,'First'], 2, 3, [4,'Fourth'], 5, ...]).

dw2 commented 8 years ago

This resolves issue #66

cdaringe commented 8 years ago

thanks @dw2! i now see what you're sayin. :)

wraithgar and I had a long thread with another user about permitting the use of mixed types. at current time, we are hesistant to support multiple option types in the options set, per discussion here. also, mixing of some types are just plain incompatible. however, we definitely do want to support numbers only. can't believe this has never come up before!

very grateful for your PR. for the time being, would you be kind enough to:

  1. add a test? they should be relatively straightforward. to imitate. see the test folder
  2. find the matching option within common types? if you want to use .find and be more functional about it, thats ok too. :)
dw2 commented 8 years ago
  1. Sure, I can add a test. I'll try to use the other tests as an example.
  2. Can you explain what you mean by "common types" and using ".find"? Do you mean adding checks for other attribute types, like booleans and dates? Something else?
cdaringe commented 8 years ago

Can you explain what you mean by "common types"

apologies for the lack of clarity. "find the matching option within common types?" should have read "adjust the code to find the matching option assuming that the options in this.options are homogeneous. e.g. 'remove support for mixed option sets'."

and using ".find"?

if you don't want to use the status quo for loop, you're welcome to find the matching option using this.options.find(function(option) { ... }). just conveying no need to maintain the status quo if you didnt care to.

dw2 commented 8 years ago

I removed support for mixed option types and added a test for numbers when using [[number','label'], ... option types. However I am unable to run the test locally because I cannot get zuul installed properly to run the test suite.

wraithgar commented 8 years ago

checked out branch as of 82ad163 and the tests pass locally for me so let's not let that be a blocker here, I'm currently trying to help doug get zuul running via other messaging channels.

dw2 commented 8 years ago

@wraithgar helped me out via Twitter DM. I can run tests locally now, but am still having separate issues with zuul. I'm taking this as a win. Thank you @wraithgar and @cdaringe for the help and feedback. Let me know if you need anything else for this PR. I hope to contribute more in the future.

dw2 commented 8 years ago

Also, thank you @jweakley for helping me troubleshoot NPM issues.