fix(uiSelectMultiple): Don't call onSelectCallback if limit already reached #1836
Conversation
|
Thanks @Jefiozie Could we instead move the call to It seems a little convoluted to have the intermediary |
|
@user378230, Agree will make some changes! Thanks for the feedback. |
|
@Jefiozie Would you mind also checking for case when Thanks! 😃 |
|
@user378230 , sure no problem. |
user378230
changed the title
|
@user378230 Have removed the selectCallBack to the directives and added a test for selectcallback without limit. |
|
@Jefiozie I think you messed up your commits here. The diff is showing changes other than yours. Force push to your existing limitOnCallback branch once you've fixed, rather than creating a new PR. |
| locals[$select.parserResult.itemName] = item; | ||
|
|
||
| $timeout(function(){ | ||
| $select.onSelectCallback(scope, { |
There was a problem hiding this comment.
Is scope the correct scope here? Is there a $select.scope it should be instead?
There was a problem hiding this comment.
I did some checking with the unit test if this is the case but then it will fail. Why do you think it should be $select.scope?
There was a problem hiding this comment.
My only thought was that it wasn't the same scope as the one in uiSelectController but I guess we don't use a new scope anyway for the uiSelectSingle/Multiple directives so it should be ok...
| @@ -1831,7 +1833,7 @@ describe('ui-select tests', function() { | |||
| </ui-select>' | |||
| ); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Check for trailing whitespace in your commits too 👍
Jefiozie
force-pushed
the
limitOnCallBack
branch
from
26e7197
to
3c89a15
Compare
| clickItem(el, 'Natasha'); | ||
| $timeout.flush(); | ||
| expect(el.scope().$select.selected.length).toEqual(2); | ||
| }); |
There was a problem hiding this comment.
Need to check the value for scope.$model.name?
| scope.$item = $item; | ||
| scope.$model = $model; | ||
| }; | ||
| var el = createUiSelectMultiple({limit:1,onSelect:'onSelectFn($item, $model)'}); |
There was a problem hiding this comment.
minor tidy up of your spacing here.. 😃
There was a problem hiding this comment.
You mean the bracket?
|
Just some tiny ones, then it LGTM 👍 |
|
Fixing it now 😄 |
…eached Previously if a limit was defined for a multiple selection ui-select and that limit was reached, the onSelectCallback would still be fired even though the item wasn't actually selected. This commit moves the firing of the callback to after the select has actually taken place to ensure it is only fired on the correct occassions. Fixes angular-ui#1794 Closes angular-ui#1836
This PR will fix the onSelect problem that was described by #1794 .