fix(uiSelectController) ActiveIndex should skip disabled choises #1848
Conversation
- "fix(uiSelectController) ActiveIndex should skip disabled choises" - angular-ui#1848
|
I think we can shorten the logic down to: ...
case KEY.DOWN:
if (!ctrl.open && ctrl.multiple) ctrl.activate(false, true); //In case its the search input in 'multiple' mode
else if (ctrl.activeIndex < ctrl.items.length - 1) {
var idx = ++ctrl.activeIndex
while(_isItemDisabled(ctrl.items[idx]) && idx < ctrl.items.length) {
ctrl.activeIndex = ++idx;
}
}
break;
case KEY.UP:
var minActiveIndex = (ctrl.search.length === 0 && ctrl.tagging.isActivated) ? -1 : 0;
if (!ctrl.open && ctrl.multiple) ctrl.activate(false, true); //In case its the search input in 'multiple' mode
else if (ctrl.activeIndex > minActiveIndex) {
var idx = --ctrl.activeIndex;
while(_isItemDisabled(ctrl.items[idx]) && idx > minActiveIndex) {
ctrl.activeIndex = --idx;
}
}
break;
... |
|
We also need a test for items being disabled due to being selected, if you wouldn't mind incorporating a test. Can you just check nothing breaks with tagging enabled and tagging label set to something and also set to Thanks! 😀 |
| switch (key) { | ||
| case KEY.DOWN: | ||
| if (!ctrl.open && ctrl.multiple) ctrl.activate(false, true); //In case its the search input in 'multiple' mode | ||
| else if (ctrl.activeIndex < ctrl.items.length - 1) { ctrl.activeIndex++; } | ||
| else if (ctrl.activeIndex < ctrl.items.length - 1) { | ||
| idx = ++ctrl.activeIndex; |
There was a problem hiding this comment.
Can we just declare the var here? I know it leaks out anyway but it makes it clearer where its used then.
| @@ -2831,6 +2833,36 @@ describe('ui-select tests', function() { | |||
| expect(scope.$model.name).toEqual('Natasha'); | |||
| expect(el.scope().$select.selected.length).toEqual(2); | |||
| }); | |||
| it('should ignore disable items key up with tagging on false', function() { | |||
There was a problem hiding this comment.
should ignored disabled items in the down direction with tagging off
| expect(el.scope().$select.activeIndex).toBe(2); | ||
| }); | ||
|
|
||
| it('should ignore disable items key up with tagging on true', function() { |
There was a problem hiding this comment.
should ignored disabled items going up with tagging on
| expect(el.scope().$select.activeIndex).toBe(-1); | ||
| }); | ||
|
|
||
| it('should ignore disable items key down', function() { |
There was a problem hiding this comment.
This is the same as tagging: false, no? Maybe we don't need this test
| @@ -3167,4 +3200,58 @@ describe('ui-select tests', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('Test key down key up and activeIndex should skip disabled choice',function(){ | |||
| it('should ignore disable items key down', function() { | |||
There was a problem hiding this comment.
should ignore disabled items, going down
| expect(el.scope().$select.activeIndex).toBe(2); | ||
| }); | ||
|
|
||
| it('should ignore disable items key up', function() { |
| expect(el.scope().$select.activeIndex).toBe(2); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Can you rename this tests too? Like how I did the others. Thank youu! 😄
|
@Jefiozie thanks for the amazing work, as always! When you have a chance can you add a test for multiple select with |
|
As requested the extra tests and changes to the idx check. |
|
@user378230 , Any ideas when this will be merged? |
|
@user378230 & @aaronroberson , any idea when this will be merged? |
|
@user378230 & @aaronroberson , any idea when this will be merged? |
|
Done! Thanks @Jefiozie |
When pressing key up or/and key down the activeIndex should be set to a item that isn't a disabled choice.
refrence: #1823