Disable tethering while tooltips are hidden #96

joukevandermaas · 2016-09-21T11:22:48Z

This was causing performance issues on pages with many tooltips, because each repositioning triggered a forced reflow (which is expensive in most browsers, but especially in IE).

joukevandermaas · 2016-09-21T13:56:41Z

I can't figure out why the build is failing, but it also fails for master when I run it locally. I don't think it's related to this PR.

a15n

good fix

a15n · 2016-10-03T22:22:37Z

addon/components/tooltip-on-element.js

@@ -336,6 +338,10 @@ export default EmberTetherComponent.extend({
    }

    this.set('offset', offset);
+
+    if (event !== 'none' || this.get('tooltipIsVisible') !== true) {


nit: !this.get('tooltipIsVisible')

a15n · 2016-10-03T22:23:37Z

addon/components/tooltip-on-element.js

@@ -430,6 +440,16 @@ export default EmberTetherComponent.extend({
    this.sendAction('onTooltipShow', this);
  },

+  stopTether() {
+    run.schedule('afterRender', this, function() {


nit: if you use arrow functions you won't need to pass in this

run.schedule('afterRender', () => { ... });

a15n · 2016-10-03T22:25:21Z

addon/components/tooltip-on-element.js

+
+  startTether() {
+    this.get('_tether').enable();
+  },


could you add tests to check that the tether is enabled when we expect it to be?

isTetherEnabled: computed.alias('_tether.enabled'),

We copied your fix here at Zenefits and did this to get test coverage

https://github.com/zenefits/ember-tooltips/pull/21/files#diff-eafc17148ee89489b3d8098122b506e6R409

sir-dunxalot · 2016-10-05T02:01:33Z

As Andrew said, this is a great fix. Good catch and thank you for putting in the time to issue the PR. I will wait until #94 has landed to merge this but the code looks good so I don't have any comment there. This will likely need a quick rebase to resolve the merge conflicts once #94 is merged onto master.

joukevandermaas · 2016-10-05T07:52:11Z

OK, I'll address @a15n's comments and rebase after #94 is merged, then. Thanks!

a15n · 2016-10-10T20:00:39Z

@joukevandermaas I think this may be helpful. It has full test coverage. I don't know if what I did with an is-tether-enabled attribute is ideal. A 'js-' prefixed class may be better.

https://github.com/zenefits/ember-tooltips/compare/v2.0.0-zenefits.1...v2.0.0-zenefits.2

joukevandermaas · 2016-10-11T07:21:01Z

I see some refactoring happened in the meantime, while adding popovers. I hope to get this PR fixed up today or tomorrow if I can.

sir-dunxalot · 2016-10-11T21:03:03Z

Sounds great, Jouke! Let myself or @a15n know if you have any questions about the refactoring from popovers.

joukevandermaas · 2016-10-12T09:07:06Z

I rebased, adapted the code and added some asserts to the visibility tests. I squashed too, so the 'old' code is no longer visible here.

I don't know why the beta build fails, but it doesn't look like it's anything to do with this PR.

a15n · 2016-10-12T14:45:45Z

tests/integration/components/tooltip-is-visible-test.js

@@ -1,6 +1,6 @@
 import Ember from 'ember';
 import { moduleForComponent, test } from 'ember-qunit';
-import { assertHide, assertShow } from '../../helpers/sync/assert-visibility';
+import { assertHide, assertShow, assertTetherEnabled, assertTetherDisabled } from '../../helpers/sync/assert-visibility';


I would recommend against creating a new assertion just for the tether. This means that people will always have to opt in to the coverage and will likely skip that test. Instead I would add the check as part of assertShow and assertHide. When I did this I found some holes in my enable/disable coverage

https://github.com/zenefits/ember-tooltips/pull/21/files#diff-c2e9fd979e3b24030663b2c4c84cd5beR3

I think this should be blocking

Yeah, good point. I'll try split it (or make a new PR if you want to merge already).

If you could do it as part of this PR that'd be great. Thanks!

a15n · 2016-10-12T14:46:26Z

addon/components/tooltip-and-popover.js

@@ -419,4 +435,20 @@ export default EmberTetherComponent.extend({
    this.sendAction('onDestroy', this);
  },

+  startTether() {
+    // can't depend on `_tether.enabled` because it's not an
+    // Ember property (so won't trigger cp update when changed)


a15n · 2016-10-12T14:49:18Z

the two test failures are expected and not related to these changes. @sir-dunxalot maybe we should make the ember-beta test failure allowable?

lvl99 · 2016-10-12T14:57:30Z

Pretty keen to see this integrated with the main build. Currently have some extreme lag after adding tooltips to my Ember app.

sir-dunxalot · 2016-10-12T19:33:10Z

Yes, I think making ember-beta failure allowable is acceptable given the conversations about the impact of glimmer (see #97). As such, this PR should be good to go once @a15n 's code comments have been addressed.

joukevandermaas · 2016-10-13T07:42:36Z

@a15n I moved the tethering assert into the existing helper. There was a slight complication, because for delayed tooltips the tethering should be enabled but the tooltip should remain hidden for a while. I added some flag to the helper now, but another potential solution is to split this specific scenario into a new helper. Let me know what you think.

a15n · 2016-10-14T00:14:02Z

@joukevandermaas thanks for your work on this.

When we did this for our local repo we didn't have to provide any additional parameters for the tests. I think that if you add the startTether() call inside the if/else within the show function you won't need the additional test parameter or special handling.

https://github.com/zenefits/ember-tooltips/pull/21/files#diff-eafc17148ee89489b3d8098122b506e6R382

joukevandermaas · 2016-10-14T09:42:44Z

Good point, I've updated the code to do that and the special assert is no longer needed. Thanks!

a15n · 2016-10-14T14:50:20Z

Great work @joukevandermaas! This code LGTM.

@sir-dunxalot ^

sir-dunxalot · 2016-10-14T15:06:39Z

+1. I will merge this, move ember-beta to allowed failures, and will release today with the passing test suite.

Great work all.

sir-dunxalot · 2016-10-14T19:57:10Z

Released as 2.3.1.

a15n reviewed Oct 3, 2016

View reviewed changes

andrew-zenefits mentioned this pull request Oct 4, 2016

As/start tether stop tether OLDZenefits/ember-tooltips#21

Merged

Disable tethering while tooltips are hidden

636063e

a15n reviewed Oct 12, 2016

View reviewed changes

a15n approved these changes Oct 12, 2016

View reviewed changes

sir-dunxalot mentioned this pull request Oct 12, 2016

Performance issues #99

Closed

Address code review comments

a7ded21

Don't have a special assert for delayed tooltips

6e26e32

sir-dunxalot merged commit 695dd16 into sir-dunxalot:master Oct 14, 2016

joukevandermaas deleted the disable-tether branch October 15, 2016 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable tethering while tooltips are hidden #96

Disable tethering while tooltips are hidden #96

joukevandermaas commented Sep 21, 2016

joukevandermaas commented Sep 21, 2016 •

edited

Loading

a15n left a comment

a15n Oct 3, 2016

a15n Oct 3, 2016

a15n Oct 3, 2016

andrew-zenefits Oct 4, 2016

sir-dunxalot commented Oct 5, 2016

joukevandermaas commented Oct 5, 2016

a15n commented Oct 10, 2016

joukevandermaas commented Oct 11, 2016

sir-dunxalot commented Oct 11, 2016

joukevandermaas commented Oct 12, 2016

a15n Oct 12, 2016 •

edited

Loading

a15n Oct 12, 2016

joukevandermaas Oct 12, 2016

andrew-zenefits Oct 12, 2016

a15n Oct 12, 2016

a15n commented Oct 12, 2016

lvl99 commented Oct 12, 2016

sir-dunxalot commented Oct 12, 2016

joukevandermaas commented Oct 13, 2016

a15n commented Oct 14, 2016

joukevandermaas commented Oct 14, 2016

a15n commented Oct 14, 2016

sir-dunxalot commented Oct 14, 2016

sir-dunxalot commented Oct 14, 2016 •

edited

Loading

Disable tethering while tooltips are hidden #96

Disable tethering while tooltips are hidden #96

Conversation

joukevandermaas commented Sep 21, 2016

joukevandermaas commented Sep 21, 2016 • edited Loading

a15n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-dunxalot commented Oct 5, 2016

joukevandermaas commented Oct 5, 2016

a15n commented Oct 10, 2016

joukevandermaas commented Oct 11, 2016

sir-dunxalot commented Oct 11, 2016

joukevandermaas commented Oct 12, 2016

a15n Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a15n commented Oct 12, 2016

lvl99 commented Oct 12, 2016

sir-dunxalot commented Oct 12, 2016

joukevandermaas commented Oct 13, 2016

a15n commented Oct 14, 2016

joukevandermaas commented Oct 14, 2016

a15n commented Oct 14, 2016

sir-dunxalot commented Oct 14, 2016

sir-dunxalot commented Oct 14, 2016 • edited Loading

joukevandermaas commented Sep 21, 2016 •

edited

Loading

a15n Oct 12, 2016 •

edited

Loading

sir-dunxalot commented Oct 14, 2016 •

edited

Loading