search

AmpersandJS / ampersand-select-view

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

Fix for #23 and other updates #32

Closed nickryall closed 9 years ago

nickryall commented 9 years ago

I had to make all of these changes at once to properly test all the use cases.

  1. fix for #23 with test
  2. return this from render method for chaining purposes
  3. beforeSubmit method ( called by form view on submit )
  4. reset method with tests ( #30 )
  5. tests for clear method
wraithgar commented 9 years ago

+1 wow this is great

lukekarrys commented 9 years ago

+1

wraithgar commented 9 years ago

@nickryall wanna take a stab at adding to the readme then I'll merge!

nickryall commented 9 years ago

Sorry guys. You might need to take another look at this. I've made some updates to prevent the UI ever getting to a state where there is an option selected but the value of the view is set to undefined.

The clear and reset methods will now fall back to selecting the first available option if there is no unselectedText or initial value set. I've updated the readme to document this behaviour.

cdaringe commented 9 years ago

@nickryall, not sure if you saw, but bear updated the travis sauce credentials. If you merge upstream/master into your feature branch and push a frivolous commit, hopefully the credentials will be zen :)

nickryall commented 9 years ago

Ok. I've made the suggested changes with a quick test included for the clear method with empty options.

I also realised that an uncaught error was thrown if no options were passed in ( expects at least an empty array or collection ) so I added a check for that.

The latest travis.yml is in the commit but still no joy.

cdaringe commented 9 years ago

hey @nickryal, your new commits look good too. this test may not be desirable per our parallel discussion. Also, I like your opts.options validation, but you're missing [[1, 2]] case, and even then, we won't verify all of the contents' validity. I think closing 5 via my PR may cover this sufficiently enough?

+1, tests passing

nickryall commented 9 years ago

@cdaringe Thanks for taking a look. Yep I'll remove that test based on the new behaviour in #36 . Perhaps it would make more sense to merge #36 first then I'll re-base and update this PR accordingly. I think some of my changes will be redundant after that anyway, so I'll need to take some time to make sure it all fits with the new direction.

wraithgar commented 9 years ago

36 is merged

nickryall commented 9 years ago

Closing this one. I'll create separate PRs for the items that are still relevant. That way we won't get a bunch of unnecessary commits.