fix(tooltip): performance and scope fixes #1455
Conversation
|
@chrisirhc I've merged the animation fix and would like to land this one as well before cutting a new release. I had a quick look at looks good to me. Do you want to do any further tweaks to this one or is it good to be merged? |
|
@pkozlowski-opensource Just the indentation, and a rebasing it so it can be merged. |
|
@chrisirhc OK. I've just pushed a new release so there is no hurry but if you could rebase this one it would help. |
|
@pkozlowski-opensource Sure, will do. Sorry about that. I'll work on backports for these fixes the 0.8.0 maintenance branch in the future. |
|
Updated with indentation fix, to ignore whitespace view with ?w=1 in the url: https://github.com/angular-ui/bootstrap/pull/1455/files?w=1 I wasn't able to reproduce #944 and check if this causes a regression on that issue, is there a test case for that? |
|
@pkozlowski-opensource This will probably have a conflict when applied before/after #1415 so let me know what that lands. |
|
@chrisirhc I've just tested it and one of those commits introduces an issue, visible on the demo page, where the tooltip on the input box is not well positioned. I saw it before, would need to dig out the exact issue and the fix. In any case we need to get to the bottom of it before this PR gets merged. Won't have time today but can investigate further in the coming days. |
|
No worries, I have an idea which line change caused it, I just needed a way to reproduce the issue so I can work on it. Shall look into it later today. |
Isolate scope contents should be the same after hiding and showing the tooltip. The isolate scope's parent should also always be set to the directive's scope correctly. Reproduces angular-ui#1191
- Calling $digest is enough as we only need to digest the watchers in this scope and its children. No need to call $apply. - Set invokeApply to false on $timeout for popUpDelay - No need to test for cached reference when tooltip isn't visible as the tooltip has no scope. Fixes angular-ui#1450 and angular-ui#1191
|
@pkozlowski-opensource Done. Small change needed (pair of parentheses heh). It was #944. |
Make sure to use a new child scope every time as watchers leak into scope. If linked DOM is removed, watchers from directives in that DOM aren't removed. Regression from angular-ui#1455
Includes the fix in #1454 as the tests in bd395ac depend on it. I opened #1454 for discussion due to its breaking changes but this commit here doesn't incur any breaking changes.
Fixes #1191
Attempts to fix #1450
Positioning is broken because at point of linking, the contents of the tooltip haven't been transcluded.
I added $digests to ensure that the tooltip's contents were transcluded after creation and linking.
Note for future feature: One thing, the linking append to the body as that allows directives from inside the tooltip to access controllers outside. This allows for templates to be embedded.
Right now the indentation isn't correct, but I'm doing this so that the changes are clear. I'll add another commit later to correct the indentation once this is reviewed.