feat(dropdown) Add keynav support (fix for #1228) #3685
Conversation
bleggett
changed the title
|
Please squash your commits. |
|
Squashed. |
| self.selectedOption+1); | ||
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
Shouldn't this be inside the case statement?
There was a problem hiding this comment.
You're saying
var elems = self.dropdownMenu ? //If append to body is used.
(angular.element(self.dropdownMenu).find('a')) :
(angular.element(self.$element).find('ul').eq(0).find('a'));should be copied to each of these case's?
Pro; code gets executed when it should
Con; duplicate code
I guess the pro is better than the con.
There was a problem hiding this comment.
Of course.
A break statement terminates execution of the smallest enclosing switch or iteration statement.
So I would vote to put these breaks inside the curly brackets as well.
|
These are mostly code style issues. @rvanbaalen if you think it's worth merging and fixing these issues before pushing up, this PR LGTM. |
| } else { | ||
| self.selectedOption = (self.selectedOption === elems.length -1 ? | ||
| self.selectedOption : | ||
| self.selectedOption+1); |
There was a problem hiding this comment.
Add spaces before and after the +.
Also make the indentation only one level deep, and not align to the self in the first line.
|
My only concern is with setting focus() on the menu item elements, I'm not sure if this would disrupt the tab flow once the dropdown is closed? The typeahead has similar functionality but is uses an activeIndex with: ng-class="{active: isActive($index) }" to decorate the element. Then listens for enter to trigger selection. |
|
@bleggett Can you incorporate these suggestions? |
|
@rvanbaalen Yep, will do. Will look at what typeahead is doing like @RobJacobs suggested as well. |
|
Can you squash your commits again? In the future, if you need to sync up with remote master, you can do |
bleggett
force-pushed
the
dropdown-keynav
branch
3 times, most recently
from
99675b8 to
a84966f
Compare
fix(dropdown): Fixed indexing corner cases and filter key events. fix(dropdown): Try using document.bind instead fix(dropdown): Add optional attrib for keyboard-nav. fix(dropdown): Dedup code and handle differences if dropdown-menu used fix(dropdown): Fix focus issue and add more tests fix(dropdown): Update docs with example fix(dropdown): Revert accidental change to misc/demo/index.html fix(dropdown): Dedup code and handle differences if dropdown-menu used Add keynav support to dropdown (angular-ui#1228) fix(dropdown): Fixed indexing corner cases and filter key events. fix(dropdown): Try using document.bind instead fix(dropdown): Add optional attrib for keyboard-nav. fix(dropdown): Fix focus issue and add more tests fix(dropdown): Revert accidental change to misc/demo/index.html fix(dropdown): Dedup code and handle differences if dropdown-menu used fix(dropdown): Update docs with example feat(dropdown): Add keynav support (fix for angular-ui#1228) feat(dropdown): Fix indentation issues and correct breaks. fix(dropdown): undo indent goofyness.
bleggett
force-pushed
the
dropdown-keynav
branch
from
a84966f to
5513a1a
Compare
|
@wesleycho Yeah, resquashed. Still getting familiar with git, so forgive the rookie mistakes. Regarding @RobJacobs suggestion: typeahead creates the menu element dynamically, and so can add attrs to it (like the select and activeIndex) relatively easily. Dropdown relies on a predefined, user-defined menu element, and I'm less sure about tacking attrs onto that blindly, or expecting users to add this stuff themselves. Am I way off here? |
|
@bleggett Thanks for looking at that, I agree with your assessment. |
Adds a new option flag to the dropdown class that enables the use of arrow keys to navigate dropdown list elements, like non-angular bootstrap does. Added tests and updated doc with example. Fix for #1228 and based off of #3212