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

Commit

Permalink
feat(typeahead): add customClass support for dropdown
Browse files Browse the repository at this point in the history
- Adds support for custom classes on the typeahead dropdown

Closes #4332
Closes #4410
  • Loading branch information
gorork authored and wesleycho committed Sep 15, 2015
1 parent 7ba2527 commit fa1cdfc
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 1 deletion.
16 changes: 16 additions & 0 deletions src/typeahead/docs/demo.html
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
<style>
.dropdown-menu.demo-class {
border-radius: 0;
font-style: italic;
box-shadow: 5px 5px 0 rgba(0, 0, 0, .12);
}
.dropdown-menu.demo-class > .active > a,
.dropdown-menu.demo-class > .active > a:hover,
.dropdown-menu.demo-class > .active > a:focus {
background-color: rgba(247, 150, 4, .69);
}
</style>
<script type="text/ng-template" id="customTemplate.html">
<a>
<img ng-src="http://upload.wikimedia.org/wikipedia/commons/thumb/{{match.model.flag}}" width="16">
Expand All @@ -21,4 +33,8 @@ <h4>Asynchronous results</h4>
<h4>Custom templates for results</h4>
<pre>Model: {{customSelected | json}}</pre>
<input type="text" ng-model="customSelected" placeholder="Custom template" typeahead="state as state.name for state in statesWithFlags | filter:{name:$viewValue}" typeahead-template-url="customTemplate.html" class="form-control">

<h4>Custom class for append-to-body popup</h4>
<pre>Model: {{customClassSelected | json}}</pre>
<input type="text" ng-model="customClassSelected" typeahead-append-to-body="true" typeahead-dropdown-custom-class="demo-class" placeholder="Custom class" typeahead="state as state.name for state in statesWithFlags | filter:{name:$viewValue}" class="form-control">
</div>
6 changes: 5 additions & 1 deletion src/typeahead/docs/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,9 @@ The typeahead directives provide several attributes:
On blur, select the currently highlighted match

* `typeahead-focus-on-select`
_(Defaults: true) :
_(Defaults: true)_ :
On selection, focus the input element the typeahead directive is associated with

* `typeahead-dropdown-custom-class`
_(Defaults: undefined)_ :
Add custom class to popup
19 changes: 19 additions & 0 deletions src/typeahead/test/typeahead.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,19 @@ describe('typeahead tests', function() {
findMatches(element).eq(1).trigger('mouseenter');
expect(element).toBeOpenWithActive(2, 1);
});

it('should add a custom class to popup', function() {
var element = prepareInputEl('<div><input ng-model="result" typeahead="item for item in source | filter:$viewValue" typeahead-dropdown-custom-class="demo-class"></div>');
changeInputValueTo(element, 'b');
expect(findDropDown(element).hasClass('demo-class')).toBeTruthy();
});

it('should add a custom class to popup with custom template', function() {
$templateCache.put('custom.html', '<div>foo</div>');
var element = prepareInputEl('<div><input ng-model="result" typeahead="item for item in source | filter:$viewValue" typeahead-dropdown-custom-class="demo-class" typeahead-template-url="custom.html"></div>');
changeInputValueTo(element, 'b');
expect(findDropDown(element).hasClass('demo-class dropdown-menu')).toBeTruthy();
});
});

describe('promises', function() {
Expand Down Expand Up @@ -895,6 +908,12 @@ describe('typeahead tests', function() {
$timeout.flush();
expect(dropdown.css('top')).toEqual('500px');
});

it('should add a custom class to append-to-body popup', function() {
var element = prepareInputEl('<div><input ng-model="result" typeahead="item for item in source | filter:$viewValue" typeahead-append-to-body="true" typeahead-dropdown-custom-class="demo-class"></div>');
changeInputValueTo(element, 'b');
expect(findDropDown($document.find('body')).hasClass('demo-class')).toBeTruthy();
});
});

describe('focus first', function() {
Expand Down
5 changes: 5 additions & 0 deletions src/typeahead/typeahead.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position'])
popUpEl.attr('popup-template-url', attrs.typeaheadPopupTemplateUrl);
}

// Add a custom class to Dropdown Menu element
if (angular.isDefined(attrs.typeaheadDropdownCustomClass)) {
popUpEl.addClass(attrs.typeaheadDropdownCustomClass);
}

var resetMatches = function() {
scope.matches = [];
scope.activeIdx = -1;
Expand Down

6 comments on commit fa1cdfc

@pkozlowski-opensource
Copy link
Member

Choose a reason for hiding this comment

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

@wesleycho @gorork I'm not sure why we've merged this one... Currently popup has the typeaheadPopup directive so people could create their own typeaheadPopup to add whatever attributes (including classes) to the pop window.

IMO this change only adds to the size of the library for a very specific case that could be solved in the user-land in a more flexible way. Could we re-consider / revert this one?

@wesleycho
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I thought this was a pretty minor addition - in terms of LOC & complexity, this feature is pretty small. To override it using a custom template requires more code for each template desired, which is why I thought this was a pretty safe thing to merge.

What are your thoughts on what problems this could cause in terms of maintenance issues down the line?

@pkozlowski-opensource
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I thought this was a pretty minor addition - in terms of LOC & complexity, this feature is pretty small.
What are your thoughts on what problems this could cause in terms of maintenance issues down the line?

I agree that this one is small and alone doesn't do much harm. The problem is more a combination of changes like those where one day we wake up with a big library with many convenience properties... and big size ... and big documentation. I've bean there, I saw libraries growing to the point that people didn't want to use them any more.

My point is that we should balance convenience with frequency. If something is rare (IMO this one is) and can be solved in the user-land, we shouldn't merge it (even if it means 5 more lines of code for users). Obviously we should monitor user requests and add things that are mostly requested, but I wouldn't merge any convenience shortcut just because it is small. Once again, frequency matters, IMO.

@wesleycho
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that is fair - I will revert this then, and keep this in mind for the future.

@pkozlowski-opensource
Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thnx @wesleycho !

@datazen
Copy link

Choose a reason for hiding this comment

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

This actually would be a NICE addition. How hard would it be just to add a class to ngb-typeahead-window element that we can style? Documentation could use more examples. You show a lot in the API section but not many examples using the methods, etc in the API part.

Please sign in to comment.