[feature] `disabled` attribute #120

knownasilya · 2016-11-29T16:41:08Z

Allows a disabled attribute to prevent showing the tooltip by any of the available methods:

{{#tooltip-on-component disabled=true}}
  My Tooltip
{{/tooltip-on-component}}

Talked about this with @sir-dunxalot on Slack.

knownasilya · 2016-11-29T16:51:28Z

Only thing I'm not sure about is not showing the tooltip if disabled=true isShown=true..

knownasilya · 2016-11-29T17:51:32Z

So I added LTS targets and those all pass along with 1.13, but delay is still broken with 2.10. This feature should still be mergeable, since this test failure is unrelated.

sir-dunxalot · 2016-11-29T18:08:57Z

@a15n I don't want to merge this without a review from you given to the effects of this on isShown. I will also do a review.

a15n · 2016-11-30T16:22:05Z

addon/components/lazy-render-wrapper.js

@@ -93,7 +94,7 @@ export default Ember.Component.extend({
 	enableLazyRendering: false,
 	_hasUserInteracted: false,
 	_hasRendered: false,
-	_shouldRender: computed('isShown', 'tooltipIsVisible', 'enableLazyRendering', '_hasUserInteracted', function() {
+	_shouldRender: computed('isShown', 'disabled', 'tooltipIsVisible', 'enableLazyRendering', '_hasUserInteracted', function() {


let's move disabled to be the first property. It is the most important.

a15n · 2016-11-30T16:23:24Z

addon/components/lazy-render-wrapper.js

@@ -93,7 +94,7 @@ export default Ember.Component.extend({
 	enableLazyRendering: false,
 	_hasUserInteracted: false,
 	_hasRendered: false,
-	_shouldRender: computed('isShown', 'tooltipIsVisible', 'enableLazyRendering', '_hasUserInteracted', function() {
+	_shouldRender: computed('isShown', 'disabled', 'tooltipIsVisible', 'enableLazyRendering', '_hasUserInteracted', function() {
 		// if isShown, tooltipIsVisible, !enableLazyRendering, or _hasUserInteracted then


can you add disabled to this note as well

a15n · 2016-11-30T16:27:28Z

addon/components/lazy-render-wrapper.js

@@ -102,7 +103,7 @@ export default Ember.Component.extend({

 			return true;

-		} else if (this.get('isShown') || this.get('tooltipIsVisible')) {
+		} else if (!this.get('disabled') && (this.get('isShown') || this.get('tooltipIsVisible'))) {

 			this.set('_hasRendered', true);
 			return true;


I can't comment below this line so see lines....

111: what if enableLazyRendering=false and disabled=true?

129: what if enableLazyRendering=false but _shouldRender=false? The didInsertElement will still apply $parent event handling and could still render the tooltip, even if it's never shown (not clean)

If these happen to be edge cases you missed can you add tests for them?

should I just handle disabled once before all of these conditionals, and return false?

And what about in 128, if i skip the parent event stuff, and then disabled gets set to false, would that be rerun anywhere else?

"should I just handle disabled once before all of these conditionals, and return false?"

I think if you add this it'll take care of _shouldRender

if (this.get('_hasRendered')) { return true; // this always needs to return true if it has ever been rendered } else if (isDisabled) { return false; ....

"And what about in 128, if i skip the parent event stuff, and then disabled gets set to false, would that be rerun anywhere else?"

You're right. If we people change isDisabled we would need to add the event listeners to the $parent. I think maybe we should make the $parent event handling a function that can be called on didInsertElement and by observing isDisabled.

We'd need good tests to cover this. It's getting fairly complex

a15n · 2016-11-30T16:56:31Z

So the disabled property will prevent the tooltip from being rendered if used on the tooltip-on-element and prevent the tooltip from being shown on tether-tooltip-on-element. It will prevent it regardless of isShown and user interaction. Am I understanding this correctly?

You can achieve the "no show" behavior if you set isShown=false and event="none" or by wrapping your tooltip in an {{#if}} block. @jliuzen how would you do this if you wanted to disable z-help-text-popover for mobile users? Assuming you had a boolean for isMobile.

I'm a little hesitant to introduce this because there are existing ways to get the same behavior and with more options comes more complexity. From a technical point of view this PR looks good though. Can you explain a little more about why a disabled property is the best option?

a15n · 2016-11-30T16:57:47Z

@aaminev would this be useful in the scenario when there is no tooltip text? You could do something like...

{{#tooltip-on-element disabled=(not tooltipText.length)}}
  {{tooltipText}}
{{/tooltip-on-element}}

This way you could disable a tooltip by explicitly depending on the length of the string.

a15n · 2016-11-30T17:00:47Z

@knownasilya also thanks for your work to add LTS support! I definitely think that's a great idea but I think it should be done in a separate PR. And also I'd prefer that we keep master green so we'd need to add 2.1.0 to the list of allow_failures until the delay test is fixed.

knownasilya · 2016-11-30T17:38:15Z

Maybe this is too big of a feature for something that could be replaced by a wrapper.. e.g.

{{#desktop-tooltip}}
  My Text
{{/desktop-tooltip}}

which just wraps the tooltip in an {{#if.

a15n · 2016-12-10T18:45:03Z

Agreed, I think the ideal way would be to create you're own wrapper.

sir-dunxalot · 2016-12-11T21:12:28Z

Another solution could be to set event='none' in scenarios where you don't want to render the tooltip.

knownasilya · 2016-12-20T18:03:44Z

@sir-dunxalot so change it to none and then change it back when you want it to show? Because wrapping it in a component doesn't really work I get no behavior unless I set tagName: '' and then I get errors:

Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at Object.clear (http://stocktonca.mg.dev/assets/vendor.js:59330:20)
    at UpdatableBlockTracker.reset (http://stocktonca.mg.dev/assets/vendor.js:59667:56)
    at TryOpcode.handleException (http://stocktonca.mg.dev/assets/vendor.js:68468:113)
    at UpdatingVMFrame.handleException (http://stocktonca.mg.dev/assets/vendor.js:68650:35)
    at UpdatingVM._throw [as throw] (http://stocktonca.mg.dev/assets/vendor.js:68363:37)
    at Assert.evaluate (http://stocktonca.mg.dev/assets/vendor.js:63745:25)
    at UpdatingVM.execute (http://stocktonca.mg.dev/assets/vendor.js:68350:24)
    at RenderResult.rerender (http://stocktonca.mg.dev/assets/vendor.js:68289:16)
    at RootState._this.render (http://stocktonca.mg.dev/assets/vendor.js:25879:18)
    at runInTransaction (http://stocktonca.mg.dev/assets/vendor.js:36838:28)

My code looks like so:

desktop-tooltip.hbs

{{#unless media.isMobile}}
  {{#tooltip-on-element side=side delay=delay duration=duration}}
    {{yield}}
  {{/tooltip-on-element}}
{{/unless}}

desktop-tooltip.js

import Ember from 'ember';
  
export default Ember.Component.extend({
  tagName: '',
  side: 'bottom',
  delay: 750
});

The error occurs when isMobile is true and the tooltip is removed.

knownasilya · 2016-12-20T19:06:02Z

Related issue: yapplabs/ember-tether#35 so this doesn't work with 2.10 if you put it in an {{#if.

knownasilya · 2016-12-20T19:24:07Z

So the workaround for that would be to wrap {{#tooltip-on-element with a <div>, but this doesn't work for me because then the tooltip doesn't show since it's on the wrong element. Looks like disabled is necessary after all, or to have to use target..

SirZach · 2017-04-13T18:03:12Z

@knownasilya was this thread continued anywhere else or is it not addressed? I see yapplabs/ember-tether#35 is still open which is blocking the upgrade to 2.11 at the moment

knownasilya · 2017-04-13T18:36:01Z

It hasn't continued. Basically, I did a wrapper component like so:

{{#unless media.isMobile}}
  <div>
    {{#tooltip-on-element side=side delay=delay duration=duration}}
      {{yield}}
    {{/tooltip-on-element}}
  </div>
{{/unless}}

SirZach · 2017-04-13T20:01:46Z

@knownasilya did you have a solution for {{tooltip-on-component}}?

knownasilya · 2017-04-13T20:31:40Z

I didn't have that requirement. I think this PR would have to be completed to support that.

Ilya Radchenko added 3 commits November 29, 2016 11:12

Add disabled option integration test

933ad99

Document disabled property

80bef41

Fix tests, implement, and add to dummy app

440426d

Ilya Radchenko added 4 commits November 29, 2016 11:56

Attempt handling isShown with disabled

e04bc87

Fix isShown support, maybe.

ccea478

Update travis and test targets

f197594

Bring back 1.13 test target and ignore beta

215ae04

sir-dunxalot assigned sir-dunxalot and a15n Nov 29, 2016

a15n reviewed Nov 30, 2016

View reviewed changes

Fix comment and observer order

cda19ae

a15n closed this Dec 10, 2016

a15n mentioned this pull request Dec 15, 2016

add LTS 2.4 and 2.8 support #132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] `disabled` attribute #120

[feature] `disabled` attribute #120

knownasilya commented Nov 29, 2016

knownasilya commented Nov 29, 2016

knownasilya commented Nov 29, 2016

sir-dunxalot commented Nov 29, 2016

a15n Nov 30, 2016

a15n Nov 30, 2016

a15n Nov 30, 2016 •

edited

knownasilya Nov 30, 2016

knownasilya Nov 30, 2016

a15n Nov 30, 2016

a15n Nov 30, 2016

a15n commented Nov 30, 2016

a15n commented Nov 30, 2016 •

edited

a15n commented Nov 30, 2016

knownasilya commented Nov 30, 2016 •

edited

a15n commented Dec 10, 2016

sir-dunxalot commented Dec 11, 2016

knownasilya commented Dec 20, 2016 •

edited

knownasilya commented Dec 20, 2016

knownasilya commented Dec 20, 2016 •

edited

SirZach commented Apr 13, 2017

knownasilya commented Apr 13, 2017 •

edited

SirZach commented Apr 13, 2017

knownasilya commented Apr 13, 2017

[feature] disabled attribute #120

[feature] disabled attribute #120

Conversation

knownasilya commented Nov 29, 2016

knownasilya commented Nov 29, 2016

knownasilya commented Nov 29, 2016

sir-dunxalot commented Nov 29, 2016

a15n Nov 30, 2016

Choose a reason for hiding this comment

a15n Nov 30, 2016

Choose a reason for hiding this comment

a15n Nov 30, 2016 • edited

Choose a reason for hiding this comment

knownasilya Nov 30, 2016

Choose a reason for hiding this comment

knownasilya Nov 30, 2016

Choose a reason for hiding this comment

a15n Nov 30, 2016

Choose a reason for hiding this comment

a15n Nov 30, 2016

Choose a reason for hiding this comment

a15n commented Nov 30, 2016

a15n commented Nov 30, 2016 • edited

a15n commented Nov 30, 2016

knownasilya commented Nov 30, 2016 • edited

a15n commented Dec 10, 2016

sir-dunxalot commented Dec 11, 2016

knownasilya commented Dec 20, 2016 • edited

knownasilya commented Dec 20, 2016

knownasilya commented Dec 20, 2016 • edited

SirZach commented Apr 13, 2017

knownasilya commented Apr 13, 2017 • edited

SirZach commented Apr 13, 2017

knownasilya commented Apr 13, 2017

[feature] `disabled` attribute #120

[feature] `disabled` attribute #120

a15n Nov 30, 2016 •

edited

a15n commented Nov 30, 2016 •

edited

knownasilya commented Nov 30, 2016 •

edited

knownasilya commented Dec 20, 2016 •

edited

knownasilya commented Dec 20, 2016 •

edited

knownasilya commented Apr 13, 2017 •

edited