fix(tooltip): hide tooltips when content becomes empty and test changes #1058
Conversation
|
@pkozlowski-opensource I was very intrigued when I saw the tooltip/popover code... in order to implement #1046 - which resolves #220 (nobody seems to have reviewed it yet...) Anyway... I've read elsewhere the "community" discussion, which mentions the best practices teaching/learning issue... I have a simple question: is this "tooltip" code a "best practices" example? Or not, as it seemed to me? I must ask because the expectation to see "only best practices" at AngularJS UI Bootstrap code is such, that I am not confident of my own judgement. Thank you. |
|
@pkozlowski-opensource I took a quick look at it; I think I'm okay with it, but I'm not sure how comfortable I am with putting a lot of DOM knowledge into the tests. If it's scope implementation vs DOM manipulation in the tests, it seems more durable to me to use the former, but I'd like to take a more detailed look over the next couple of days. |
|
@joshdmiller I totally hear what you are saying. At the end of the day we want tests to cover as much details as possible and have as solid and change-proof as possible. So there is natural conflict here and there are seldom perfect solutions. The real challenge is in guessing which parts are likely to change. Both CSS can change (with BS3) as well as scoping (with upcoming refactorings). So yes, it is a bit of betting on the future which is always a tricky business.... Having said the above I feel like testing through the "public API" of the directive (which is its HTML markup that triggers the directive as well as resulting markup) gives us greater freedom in refactoring internals (which, once again, we need to do for the upcoming changes - for example - I was thinking of using transclusion in tooltip / popover templates and this is probably going to disturb current scoping assumptions). Anyway, thoroughly enjoying this conversation, I don;t think there is a perfect solution here, open to any suggestions / comments. |
|
@jbruni I'm traveling right now so can't spend much time on this project - your PR will have to wait for the review. But I've started to look into the tooltip code to learn it better as we want to support templates for popovers, this is for sure. Now, as for "best practices" - I think that @joshdmiller did great job with the tooltip / popover directives and you can learn a lot from reading this code. There are some clever techniques used that you will hardy find anywhere else. But "best practices" are nothing more than a community wisdom captured in coding patterns. And this "wisdom" doesn't come from the thin air - it is based on practical use-cases. As such best practices evolve and this is good. In short - I think that this project is an amazing learning resource but take everything with a grain of salt. Best practices do evolve and our knowledge evolves as well. Some of the code in this project is almost 1-year old and this is huge for the very young framework like AngularJS. |
|
@pkozlowski-opensource Thanks a lot for the carefully thought and written feedback. I will wait for the PR review. Meanwhile, I announced it in the popover template support issue so others can also test and/or review. I apologize for my previous comment. I recognize I may have sounded rude and arrogant. Sorry. I should have measured my words better before publishing them. |
|
@jbruni no worries. @joshdmiller did you have a chance to have a deeper look at my commit and comments above? My main motivation here is to be able to move towards support of programmatic triggers and content from a template. This, IMO, will require changes to the internal structure of the tooltip and as such I would like to have tests that are based on the "public API" and as black-box as possible. Open to any suggestions. |
|
@joshdmiller I haven't heard from you so decided to merge it as cf5c27a since it solves the reported and valid issue. If you don't like tests we can delete them, I've got no problem here. |
@joshdmiller @angular-ui/bootstrap I was looking today into #875 and realized that I've got a bit of hard time testing new functionality. So I've looked a bit more into existing tests and realized that those are gray-box tests in many cases. I mean, the tests assume a lot about internal implementation, including scope management.
I think that there are some bigger changes on the horizon for the tooltip / popover (mostly to cover #220 and programmatic triggers) so I would prefer that we move more into black-box testing where we can easily change internal structure of directives without breaking all the tests.
Let me know what do you think about the way of writing tests proposed here.