-
Notifications
You must be signed in to change notification settings - Fork 6.7k
feat(dropdown): support programatic trigger, toggle callback & esc key #1616
Conversation
@angular-ui/bootstrap Anyone willing for a code review? This is basically a refactor, but I tried not to introduce any breaking change. The idea is to make the |
Took a quick look. Will read in more detail later tonight. How about using the element's blur event instead? And perhaps triggering blur on escape? |
}); | ||
|
||
it('should close on $location change', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have the need to test that isopen
is false when the event is triggered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is removed. This test is moved at line 67.
👍 for me. Extra stuff that is useful, non-breaking and with no overhead. Why not? :) |
@chrisirhc This seems reasonable, I need to investigate though :-) |
@chrisirhc Checked with bootstrap and they also use document click to hide the menu. This way you can navigate on the page with tab without closing the dropdown. Is there any specific issue you see that we should not use document click? |
} | ||
}; | ||
|
||
var documentClickBind = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly rename this to closeDropdown
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably I should extract the
openScope.$apply(function() {
openScope.isOpen = false;
});
into a closeDropdown
function and call from the bindings.
Ah you're right, somehow I had the impression that tab should dismiss the dropdown toggle, but I was wrong. |
}; | ||
|
||
this.toggle = function() { | ||
$scope.isOpen = !$scope.isOpen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should take in an argument open
and check if arguments are 0:
if (arguments.length) {
return $scope.isOpen = !!open;
} else {
return $scope.isOpen = !$scope.isOpen;
}
This way it can handle an argument that sets the open explicitly and it also returns the actual value after the toggle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's nice, but there was no use case for this. See test coverage report :-D
Do you want to add as future proof solution in case someone wants to hook on the controller, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bekos I like @chrisirhc's idea. Use-case: Perhaps there's a tutorial where it says "When you click here, this dropdown will open!".
and you want to make sure it actually stays open and doesn't close if the user already had it open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajoslin The toggle
method is not exposed to the directive. It is only used by the dropdown toggler to do the toggling.
I have added in the demo a button that does what you say, by just setting the is-open
to true.
I think @chrisirhc is wanting this in case another directive wants to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make the change anyway :-)
Thanks for the review! I will change the issues mentioned and merge it. I would also rename the module from |
Landed on master. |
No description provided.