Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for number type attributes #68

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dw2
Copy link

@dw2 dw2 commented Feb 9, 2016

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
Copy link
Author

dw2 commented Feb 9, 2016

This resolves issue #66

@cdaringe
Copy link
Member

cdaringe commented Feb 9, 2016

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
Copy link
Author

dw2 commented Feb 9, 2016

  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
Copy link
Member

cdaringe commented Feb 9, 2016

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
Copy link
Author

dw2 commented Feb 9, 2016

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
Copy link
Contributor

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
Copy link
Author

dw2 commented Feb 9, 2016

@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
Copy link
Author

dw2 commented Feb 9, 2016

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

}
} else {
throw new Error('no options provided');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like and support this, we tend to want to throw exceptions when devs get themselves into places that would give unexpected results. This will mean a major release though because it would not have thrown in this case before.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think I should remove this for now and do a separate PR for it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! Leave it. Versions are cheap I'm just making sure we see that so we don't miss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants