Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(typeahead): add 'is-toggled' option #4779

Closed
wants to merge 3 commits into from

Conversation

chenyuzhcy
Copy link
Contributor

This is related to issue #4760

@RobJacobs
Copy link
Contributor

@chenyuzhcy I'm assuming you are adding a feature to indicate whether the typeahead dropdown is open or not? In all the other directives that offer this feature we have been using 'is-open', so for consistency sake, could we name this the same?

expect($scope.isToggled).toBeFalsy();
changeInputValueTo(element, 'b');
expect($scope.isToggled).toBeTruthy();
var match = $(findMatches(element)[1]).find('a')[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to do var match = findMatches(element).find('a').eq(0)

@wesleycho
Copy link
Contributor

This LGTM, and I'm fine merging while making the changes as merging - any objections here?

@wesleycho wesleycho added this to the 1.0.0 milestone Oct 29, 2015
@chenyuzhcy
Copy link
Contributor Author

@RobJacobs I had the same idea at first but then I looked all the 'is-open' in the other directives, it all does toggle to the dropdown, but this is only an indicator of open or not (it does not manually open the dropdown even we set it to true) so I chose the name that's mentioned in the issue. But if you still insist 'is-open' is a better name I can change it very quickly.

@wesleycho
Copy link
Contributor

I think is-open is a better name - I think the semantic is off in the other directives, but too late for those.

@wesleycho wesleycho closed this in 167cfad Oct 29, 2015
@chenyuzhcy chenyuzhcy deleted the #4760 branch October 29, 2015 23:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants