reset and beforeSubmit Methods. Explicitly hide message container on setup.

[nickryall]

1. Tests for clear method.

2. Added the reset method. Works as follows:

   • If a starting value is available reset to that.
   • If there is no starting value fall back to the first option. Note: This will be unselectedText if it is available or null if for some reason there are no options. Otherwise it's just the value of first option within the select element.

   @cdaringe I was going to fallback to clear here, however, it will result in an error if there isn't a null option available. Is this the expected behaviour of clear?

3. Explicitly hide the message container on setup. Currently the validation message will be displayed on setup unless you hide it explicitly in your own code. Setup of the view should handle this. Tests included.
4. beforeSubmit method with test. Used by FormView. Ensures the value of the selected option is set on the view when the form is submitted, triggering any validation. See the related test for expected behaviour.

[cdaringe]

hey @nickryall:

• Given that we now know that startingValue must exist in the option set, reset() should probably require no other logic than to setting the value back to startingValue and then updating the selected option in the control (setValue already handles the latter). perhaps if the option set is dynamic and the starting option is removed, other logic could justified. however, in that case, I would lobby that it's not truly a reset() anymore. what do you think?

I was going to fallback to clear here, however, it will result in an error if there isn't a null option available. Is this the expected behavior of clear?

• I interpret clear as removal-of-value. that would mean to set the field value to null, or perhaps the the empty string (not undefined, tho!). admittedly, however, the case of the empty string still isn't quite baked in SelectView. we should certainly do so soon. options: ['a', '', 'c'] then a .setValue on '', for example, will fail.

[nickryall]

@cdaringe

I think of reset as setting the view back to the state it was in directly after initialization. So if there is a startingValue it will be reset to that, otherwise it will be set to the default state which is normally the first available option selected. Similar to how resetting a form with an button type="reset" would work.

[cdaringe]

hey @nickryall. sure, but if you can only ever have an option in the option set, then there is always a startingValue

[nickryall]

@cdaringe Apologies for the confusion. By startingValue I'm referring to what is passed in with opts.value, which not always be available. Yes you are right, there will always be an available value, which is what it falls back to if opts.value is not available.

[cdaringe]

apologies for the lack of clarity in my original intent, as the remark didn't reflect your PR code! hence the confusion

[nickryall]

No probs :)

Also, the explanation of clear makes sense. Thanks.

[cdaringe]

too funny--we're on the same page. initializing startingValue unconditionally on construction would be some extra effort, but may be worth it long term. we have the following case that could create issue:

• you may have a collection of Models with ids 'foo', 'bar', and 'baz'
• no initial value is passed to the constructor
• post-initialization, 'foo' gets removed.
• a reset happens. 'bar' is now selected

I'd think that's an Error state, as really, 'foo' was the original value. Regardless, I think what you've got here now is good, and definitely beats nothing.

let me know if you'd like to arm-wrestle some more over clear. a 0th-index selecting clear() may feel right to others beyond yourself

[nickryall]

Yeah, I think the behaviour of having options being added and removed post construction is a tricky one to solve.

The only way to get around your example above, other than throwing an error, would be to have the reset method add the foo model back to the collection, which I don't think is the responsibility of the view in this instance.

Also at present, if an initial value is passed and the option with the same value is later removed, this error is thrown. 'Error: no option exists for value'.

My opinion is that the view should adjust to these external changes in the collection by falling back to the first available option. This will prevent unforeseen errors when changes in the underlying collection occur and mean that what is displayed in the UI and the current value are the same.

In reality though, I think it's going to be an edge case where a select view is rendered then changes happen in the collection during the lifecycle of the view. So perhaps it's something that devs can handle in their own code. Perhaps a group discussion in Gitter would be beneficial.

[cdaringe]

@nickryall. i hear ya. Furthermore, to support your thought process, I think that's what @latentflip may have originally intended as the lib seemed to like defaulting to index 0. My opinion is still that a bulletproof form won't passively accept an error state. If I start in an arbitrary state A, request to reset() to state A, yet end up in state B--that's contrary to the linguistic intent, thus arguably an error. Passively moving to state B I think may induce user heartache in the future. Perhaps we make this configurable?

@wraithgar @kamilogorek @latentflip we'd like your input on this discussion. To summarize briefly, the discussion is simply should a reset() in ampersand-select-view default to the 0th-index or should it throw an Error, if and when the original value is no longer present in the option set.

[wraithgar]

As far as the reset question, I'm afraid I'm gonna have to reply w/ a question of my own. How are people setting the starting selected option in a select-view currently?

[nickryall]

@wraithgar Thanks for the review. At present I think the only way to have a starting selected option is to pass a starting value as opts.value through the constructor. If there is a corresponding option with the same value then that is selected. I don't think there is a way to simply set the selected property on an particular option.

[wraithgar]

Ok thanks. If they pass a value, and on reset() that value is gone it should throw. If we want to cover the case where the app knows that the value could be gone and doesn't want an exception we can add a flag to the reset() function that prevents the exception and falls back to index 0.

[nickryall]

Ok thanks guys. I've removed the fallback for reset and cleaned up the other areas. If we decide to cover the fallback case later I can create a new PR :)

[cdaringe]

hey @nickryall:

still lovin' this PR. I hope I'm not coming off rude or too picky :smile: , but...

1. reset doing nothing when opts.value isn't defined doesn't really solve my initial problem statement. i'm still thinking that reset needs to be honest, and needs to be explicit. Perhaps we could adjust setValue() to return the actual value vs this (no chaining :( ), and then in construction we modify the setValue call to look like this.startingValue = setValue(opts.value, opts.eagerValidate ? false : true);. To finish it up, reset would be simply be function() { return this.setValue(this.startingValue, true); }. It would break some of your tests as they stand, but at least we'd know explicitly what the startingValue really is, and reset would try to set it to it no matter what.
2. beforeSubmit won't work as you have it with collections, as yieldModel, for instance, expects an object to be passed to setValue. In your case, you're just submitting a string.

I checked out your PR and did an update on it. Solving the first issue actually led to finding some further issues, hence the ~large update. Check out the diff here.

Tell me what you think!

[nickryall]

1. I think I'm getting little confused here now ;) More than happy to go with your implementation :)
2. I haven't made any changes to clear, so I'm not sure what is happening there?
3. I've tested and beforeSubmit with yieldModel: true and a collection works as expected. This is because setValue calls getOptionByValue which then will call getModelForId if a model isn't found using the value directly. So I think the extra check in onChange is redundant at present. Although its probably best to catch it earlier in both cases.

Anyway, The diff looks good to me and solves some other issues so please use. Thanks!

On Wed, Apr 1, 2015 at 9:34 PM, Christopher Dieringer < notifications@github.com> wrote:

hey @nickryall https://github.com/nickryall:

still lovin' this PR. I hope I'm not coming off rude or too picky [image: :smile:] , but...

1. reset doing nothing when opts.value isn't defined doesn't really solve my initial problem statement. i'm still thinking that reset needs to be honest, and needs to be explicit. Perhaps we could adjust setValue() to return the actual value vs this (no chaining :( ), and then in construction we modify the setValue call to look like this.startingValue = setValue(opts.value, opts.eagerValidate ? false : true);. To finish it up, reset would be simply be function() { return this.setValue(this.startingValue, true); }. It would break some of your tests as they stand, but at least we'd know explicitly what the startingValue really is, and reset would try to set it to it no matter what.
2. Did we bottom out on clear? Clear still just set's the 0th index. In many value sets, the 0th index generally has no special meaning contrasted against than the nth index, unless the 0th is unselectedText. unselectedText has null view value, thus I still think a clear should set null.
3. beforeSubmit won't work as you have it with collections, as yieldModel, for instance, expects an object to be passed to setValue. In your case, you're just submitting a string.

I checked out your PR and did an update on it. Check out the diff here https://gist.github.com/cdaringe/2aa1dac555a0a6cb4c79.

Tell me what you think!

— Reply to this email directly or view it on GitHub https://github.com/AmpersandJS/ampersand-select-view/pull/39#issuecomment-88393623 .

[nickryall]

Hey Sorry, I initially answered from email. The updates look great. Yes of course reset should fall back to the initial starting value even if opts.value was not passed. This looks good now :) I also like the improved handling of 'add reset remove' from the collection.

[cdaringe]

How about I tidy up my content, and submit a PR to your personal master branch? In that regard, you can see more clearly my take on your good stuff, and we can keep it in this PR. I'm thinking about trashing the try blocks for performance reasons and using a test-if-option-exists sort of implementation.

I meant to trash that clear remark. Sorry there!

[nickryall]

Sure sounds good. Whatever works best :)

[cdaringe]

@nickryall, give it up for teamwork :metal: !

+1, tests pass

[wraithgar]

+1 we should ship this and #43 together right?

[cdaringe]

yep, that'd be ideal