This repository has been archived by the owner on May 29, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat(typeahead): add customClass support for dropdown
- Loading branch information
Showing
4 changed files
with
45 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fa1cdfc
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.
@wesleycho @gorork I'm not sure why we've merged this one... Currently popup has the
typeaheadPopup
directive so people could create their owntypeaheadPopup
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?
fa1cdfc
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.
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?
fa1cdfc
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 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.
fa1cdfc
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.
Ok, that is fair - I will revert this then, and keep this in mind for the future.
fa1cdfc
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.
Awesome, thnx @wesleycho !
fa1cdfc
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 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.