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

Fix tooltip continuation #4418

Closed
wants to merge 2 commits into from
Closed

Fix tooltip continuation #4418

wants to merge 2 commits into from

Conversation

bretkikehara
Copy link

Deleted branch instead of doing a rebase #4408 branch, so I can't reopen that PR. Sorry! >_<

I have rebase the code.

bret.ikehara added 2 commits September 14, 2015 22:48
Prefer to evaluate async over calling the apply method.

Issue: #3557
Do not recreate tooltip as this will add unnecessary processing to revive tooltip from the grave.

Issue: #3557
@bretkikehara
Copy link
Author

@wesleycho I am having trouble replicating the tests failures on Travis on my own computer. Travis shows that there should be 27 failures, but my computer prematurely disconnects. Any help would be great, so I can fix up any tooltip/popover tests.

Here is an example of the terminal when I run grunt on the master branch:

15 09 2015 00:21:29.999:INFO [karma]: Karma v0.13.9 server started at http://localhost:9876/
15 09 2015 00:21:30.006:INFO [launcher]: Starting browser Chrome
15 09 2015 00:21:31.884:INFO [Chrome 45.0.2454 (Mac OS X 10.10.5)]: Connected on socket NIyEYZhC7M89JtGlAAAA with id 90771572
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 4.875 secs: datepicker directive   value is correct
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 0.118 secs: datepicker directive   moves to the next month & renders correctly when `next` button is clicked
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 0.102 secs: datepicker directive   updates the model only when a day is clicked in the `next` month
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 0.19 secs: datepicker directive   should not "jump" months
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 0.103 secs: datepicker directive   time zone bug should not change hours if time zone bug does not occur
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 2.046 secs: datepicker directive   when `model` changes not to a Date object to a Number, it updates calendar
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 0.131 secs: datepicker directive   when `model` changes not to a Date object to a string that can be parsed by Date, it updates calendar
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 0.173 secs: datepicker directive   month selection mode renders correctly when a month is clicked
Chrome 45.0.2454 (Mac OS X 10.10.5) SLOW 0.119 secs: datepicker directive   year selection mode moves to the next year set when `next` button is clicked
Chrome 45.0.2454 (Mac OS X 10.10.5): Executed 210 of 955 SUCCESS (0 secs / 13.213 secs)
Chrome 45.0.2454 (Mac OS X 10.10.5): Executed 210 of 955 DISCONNECTED (25.996 secs / 13.213 secs)
Warning: Task "karma:continuous" failed. Use --force to continue.

Aborted due to warnings.

@Foxandxss
Copy link
Contributor

Don't touch anything while you run the test locally. For some reason, karma is very brittle sometimes and if you move to another desktop or you hide the console and chrome window, it disconnect and fails.

@RobJacobs
Copy link
Contributor

Keep in mind, Travis is using Firefox when running the test. You may want to try switching to Firefox locally to see if you get the same results by switching the following line in the Karma.conf.js:

browsers: ['Firefox'],

@wesleycho
Copy link
Contributor

I see the failures on Chrome too, so I think it's browser agnostic.

@bretkikehara
Copy link
Author

Thank you all the help. I was able to successfully replicate the Travis CI errors by switching the tests to Firefox.

@RobJacobs
Copy link
Contributor

I've been giving this change some thought and while it makes sense to implement the eval/apply async pattern, we need to take a step back and analyze what we are trying to accomplish with kicking off a digest cycle. From my understanding, in the show and hide methods, the isOpen property is getting toggled and that is the only local scope value getting changed. Since this action is occurring outside of an Angular cycle (button click, mouse over, etc) the scope digest needs to be triggered manually for other components (popover) to recognize the isOpen property has changed.

The primary difference I see with the current implementation and the suggested change is:

current: If a root scope digest cycle is not already in progress, trigger a tooltip scope digest cycle.
suggested: Schedule a tooltip scope digest.

I can see where the current implementation would have the advantage of not triggering a digest unless it's necessary where the suggested change will always trigger a digest.

It seems most of the tests are failing around not triggering a scope.$digest() after an element trigger.

@wesleycho
Copy link
Contributor

I'm not against us making a major change in the tests if necessary.

Are you interested in possibly helping out here @RobJacobs and working on top of these commits to fix the testing issues?

@RobJacobs
Copy link
Contributor

@wesleycho Will do

@RobJacobs
Copy link
Contributor

@wesleycho In working through this change, I'm seeing some issues with the positioning changes made under PR #4363. This can be viewed on the demo page- tooltip section 'dynamic' implementation and 'delayed' implementation. Setting width/height in the position routine seems to muck with arrow placement when tooltip position is top. Not sure if you want that fix as part of this PR or a seperate PR?

@wesleycho
Copy link
Contributor

Separate PR if possible just to keep it clean.

@RobJacobs
Copy link
Contributor

@wesleycho @bretkikehara I have a PR on my fork with the tests fixed and slight change to the implementation. Let me know how you want to proceed, thanks.

@wesleycho
Copy link
Contributor

Open a new pull request with everything squashed.

@bretkikehara
Copy link
Author

@RobJacobs Thank you for all your work. Looks good!

If I may make a suggestion, couldn't hideTooltipBind and hide be merged into 1 function?

jasonaden pushed a commit to deskfed/bootstrap that referenced this pull request Jan 8, 2016
This is a rollup commit intended to address several
issues around the positioning and parsing of
attributes.

- Fixes issue introduced under PR angular-ui#4311 where setting
  height and width in tooltip position function
  messed up arrow placement.
- Fixes issue introduced under PR angular-ui#4363 where setting
  visibility to hidden in tooltip position function
  caused elements in popover to lose focus.
- Fixes issue angular-ui#1780 where tooltip would render if
  content was just whitespace.
- Fixes issue angular-ui#3347 where tooltip isolate scope was
  being accessed after it was set to null.  Observers
  will now be created/destroyed as tooltip opens/closes
  which will also offer a performance improvement.
- Fixes issue angular-ui#3557 by implementing evalAsync to set
  tooltip scope isOpen property.
- Fixes issue angular-ui#4335 where if model isOpen property is
  undefined, tooltip would call show/hide toggle function.
- Closes PR angular-ui#4429 where how the templated content
  was being evaluated could cause an infinite digest loop.

Closes angular-ui#4400
Closes angular-ui#4418
Closes angular-ui#4429
Closes angular-ui#4431
Closes angular-ui#4455

Fixes angular-ui#1780
Fixes angular-ui#3347
Fixes angular-ui#3557
Fixes angular-ui#4321
Fixes angular-ui#4335
jasonaden pushed a commit to deskfed/bootstrap that referenced this pull request Jan 8, 2016
This is a rollup commit intended to address several
issues around the positioning and parsing of
attributes.

- Fixes issue introduced under PR angular-ui#4311 where setting
  height and width in tooltip position function
  messed up arrow placement.
- Fixes issue introduced under PR angular-ui#4363 where setting
  visibility to hidden in tooltip position function
  caused elements in popover to lose focus.
- Fixes issue angular-ui#1780 where tooltip would render if
  content was just whitespace.
- Fixes issue angular-ui#3347 where tooltip isolate scope was
  being accessed after it was set to null.  Observers
  will now be created/destroyed as tooltip opens/closes
  which will also offer a performance improvement.
- Fixes issue angular-ui#3557 by implementing evalAsync to set
  tooltip scope isOpen property.
- Fixes issue angular-ui#4335 where if model isOpen property is
  undefined, tooltip would call show/hide toggle function.
- Closes PR angular-ui#4429 where how the templated content
  was being evaluated could cause an infinite digest loop.

Closes angular-ui#4400
Closes angular-ui#4418
Closes angular-ui#4429
Closes angular-ui#4431
Closes angular-ui#4455

Fixes angular-ui#1780
Fixes angular-ui#3347
Fixes angular-ui#3557
Fixes angular-ui#4321
Fixes angular-ui#4335
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants