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

feat(tooltip): added tooltip-html-unsafe directive #257

Closed
wants to merge 1 commit into from

Conversation

joshdmiller
Copy link
Contributor

The directive displays the unsanitized HTML in the tooltip instead of
the escaped text.

The $tooltip service has been modified to allow a little more
flexibility in terms of the prefix used on the $observe'd attributes.
For example, the tooltip-html-unsafe directive needs to be called as
written, but it would be nonsensical to require all other attributes
(like animation or placement) to also use that verbose prefix as opposed
to the simpler and more familiar tooltip- prefix. The service now
allows independent specification of the name and its prefix.

Lastly, the docs for the tooltip and popover have been updated to show
their available optional attributes.

Closes #246

The directive displays the unsanitized HTML in the tooltip instead of
the escaped text.

The $tooltip service has been modified to allow a little more
flexibility in terms of the prefix used on the $observe'd attributes.
For example, the `tooltip-html-unsafe` directive needs to be called as
written, but it would be nonsensical to require all other attributes
(like animation or placement) to also use that verbose prefix as opposed
to the simpler and more familiar `tooltip-` prefix. The service now
allows independent specification of the name and its prefix.

Lastly, the docs for the tooltip and popover have been updated to show
their available optional attributes.

Closes angular-ui#246
@pkozlowski-opensource
Copy link
Member

Great stuff @joshdmiller! Looks good on the first sight, wanted to see if tests pass on all browsers but seems like Jenkins is not pinking up git changes... Will try to ping Igor to see what is going on here, we need to hold on non-trivial merges till this is resolved.

@pkozlowski-opensource
Copy link
Member

@joshdmiller looks like CI is back so I've merged it into the merge257 branch, tests on all browsers are OK:
http://ci.angularjs.org/job/angularui-bootstrap/276/

Added some comments to the 17be39b but those might be more like me having hard time reading the code. Feel free to comment / change / ignore. Otherwise good to merge.

Solid wrok, the $tooltip service proves its usefulness!

@joshdmiller
Copy link
Contributor Author

@pkozlowski-opensource Awesome. The dump was cruft. I'll remove it. Sorry about that. For the span, you're probably right, but since they're nested, the one we want will always be the first one anyway. I'll change the inserted span to a b or something when I remove the cruft to make it more clear.

As for the single test, you are right that I do not test to ensure HTML is output. The reason for this is that this directive actually doesn't do it - the ngBindHtmlUnsafe directive does. This directive uses a scope variable on the outer directive called tt_content, which we pass into the isolate sub-directive as content, which is passed in as an interpolated value to ngBingHtmlUnsafe. Other (existing) tests ensure that the value passed to tooltip makes its way into the final variable.

So while I didn't write a test that the HTML itself makes it out, all this would really test is that (a) the template contains the string ng-bind-html-unsafe="{{content}}", which isn't something I feel should be tested; and (b) that ngBindHtmlUnsafe is working correctly, which really should be an assumption.

The alternative would be to test that the contents of the inserted DOM node matches this HTML, but that would require building in knowledge of the template into the tests, which I think we should avoid if possible. Plus I don't think it's necessary anyway.

So the only test I wrote is that the directive actually gets called - with the service and provider, everything else should already have been tested.

What do you think?

@pkozlowski-opensource
Copy link
Member

@joshdmiller don't worry about the dump, this is what code reviews are for!

Regarding the tests:

  • yeh, changing <span> to <b> or, even better, <strong> would make this test more readable and explicit IMO
  • as for not testing the actual output - I hear what you are saying but personally I like to tests things from the "public API" perspective. Here the public API is... markup (both for input and output) so I would test that the actual tag is being rendered. I understand what you are saying about not testing AngularJS tags, but IMHO it is assuming too much about internals of the implementation. Also I wouldn't worry about relaying on the markup in a template - we do it all over the place in other tests and frankly this gives me the peace of mind - I can be sure that the expected thing gets rendered.

Anyway, there are many ways of writing tests so feel free to ignore the above, just sharing my thoughts here.

Otherwise go ahead and merge it!

@ajoslin
Copy link
Contributor

ajoslin commented Mar 28, 2013

This looks good to me :)

1 similar comment
@ajoslin
Copy link
Contributor

ajoslin commented Mar 28, 2013

This looks good to me :)

@pkozlowski-opensource
Copy link
Member

@joshdmiller I would like to land it and work toward a next release. Do you want me to do the changes discussed in this PR and merge it? Or are you going to merge it?

@joshdmiller
Copy link
Contributor Author

@pkozlowski-opensource Somehow this got knocked out of my queue. I'll fix and merge today.

@pkozlowski-opensource
Copy link
Member

@joshdmiller awesome, I will work on other PRs / issues then.

@joshdmiller
Copy link
Contributor Author

Landed as 45ed280.

@joshdmiller joshdmiller closed this Apr 3, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add tooltip-html-unsafe directive
3 participants