feat: add delay prop to Tooltip by alisd23 · Pull Request #143 · reactstrap/reactstrap

alisd23 · 2016-09-15T10:28:49Z

Previous delay was a harcoded value of 250ms for the hide, and no delay for the
show.
New prop delay allows for either an object of form:
{ show: 100, hide: 200 } or simply a number to set these delays.
Default is { show: 0, hide: 250 }

Closes #115

TheSharpieOne · 2016-09-15T13:50:14Z

  toggle: PropTypes.func,
-  children: PropTypes.node
+  children: PropTypes.node,
+  delay: PropTypes.oneOfType([PropTypes.object, PropTypes.number])


Why not use PropTypes.shape to define the object instead of just the generic object since you are mandating that show and hide have to be a number in getDelay. It avoids the NaN check and would give developers a specific message when they don't use the right shape.

If you do not use shape and keep the NaN check, then you should probably add a test for when something other than a number is passed.

Speaking of tests, I don't really see one that tests what happens when only hide or show(not both) are in the object. The code does account for this scenario, but the tests do not ensure it works (in the future, someone may break this and without tests, no one will know)

Yeah good idea, I'll use shape instead. And I can add a couple of tests to check that the hide or show defaults are picked up 👍

TheSharpieOne · 2016-09-15T13:52:27Z

-  onTimeout() {
-    if (this.props.isOpen) {
-      this.toggle();
+  getDelay(key) {


Instead of running this on every mouse over and mouse leave, why not do this when it mounts and each time props update. It will help avoid overhead.

It's true that that would reduce some overhead, but I feel the tiny performance increase would not be worth complicating the code with extra lifecycle logic.

It's only a couple of direct accesses from the props and an if statement, and mouseover and mouseleave events aren't realistically going to happen very often unless the user is flailing the mouse around.

TheSharpieOne · 2016-09-15T13:57:34Z

+        clearTimeout(this._hideTimeout);
+      }
+      if (this._showTimeout) {
+        clearTimeout(this._showTimeout);


You need to clear out this.showTimeout and this.hideTimeout. You can simply set them to undefined. Without clearing them out, they will still be truthy and thus, every click after the have been set will trigger the clearTimeout, even if the timeout has already been cleared.

If fact, everywhere you use clearTimeout you will have this problem. It needs to clear the timeout and unset the var that contains the timeout id (this._showTimeout and this.hideTimeout).
Also, when the timeout triggers the function (show/hide) they need to clear the var containing their respective timeout id (this._showTimeout and this.hideTimeout respectfully).
It would probably be useful to have a helper method to do this. like clearShowTimeout and clearHideTimeout since this code is already duplicated in a few places.

Yeah that's true, it would run clearTimeout unnecessarily. That must have already been an issue before this PR though, because there was no unsetting of the timeout property either. But I may as well include that change in this PR just to tidy it up.

TheSharpieOne · 2016-09-16T15:48:33Z

When delay={{ show: 200 }}, hide would be undefined from getDelay. This is probably not wanted.
Testing this is the test I was previously talking about.

alisd23 · 2016-09-16T16:03:39Z

Well now the propType for delay is shape({ show: number, hide: number }), there will be a warning if the user doesn't supply both. Is that not enough? I think you were right before, it's better to tell the dev their input is wrong than hide it and choose a value for them.

TheSharpieOne · 2016-09-16T18:10:01Z

I think the shape will ensure that there are no extra properties/the properties are of the type defined, but it will not ensure that the properties are there. You have to add isRequired to ensure they are there. Something like http://stackoverflow.com/a/26924696/1873485

Edit: Sorry, I didn't mean to indicate that this should be done here, but it illustrate the difference between defining the shape and mandating the shape by showing how to require the props.

Previous delay was harcoded value of 250ms for the 'hide', and 0 for the 'show'. New prop 'delay' allows for either an object of form: { show: 100, hide: 200 } or simply a number to set these delays. Default is { show: 0, hide: 250 } Closes reactstrap#115

alisd23 · 2016-09-17T15:43:08Z

@TheSharpieOne Ok I think this is ready

eddywashere · 2016-09-17T17:27:50Z

If { show: 0, hide: 250 } is the default, I would assume setting {hide: 0} would result in: { show: 0, hide: 0 }. It doesn't need to be part of this pr since adding it wouldn't introduce a breaking change.

eddywashere · 2016-09-17T17:31:15Z

+      expect(isOpen).toBe(false);
+    });
+  });
+


this comment mentions adding a test here for partial delay object. It's fine to add a test case that expects an error, and then fix in another pr or grab the value from the default object which I'd like to eventually be the way this works.

Yep OK fair enough, it should be a quick fix so I'll just get it in this PR.

eddywashere · 2016-09-17T17:32:24Z

thanks for working through this @alisd23 & @TheSharpieOne

TheSharpieOne · 2016-09-17T21:14:18Z

  tether: PropTypes.oneOfType([PropTypes.object, PropTypes.bool]),
  // optionally overide tether config http://tether.io/#options
+  delay: PropTypes.oneOfType([
+    PropTypes.shape({ show: PropTypes.number.isRequired, hide: PropTypes.number.isRequired }),


Tiny thing. Docs still showing it was required.

Damn good catch

TheSharpieOne

LGTM

eddywashere

👍

TheSharpieOne reviewed Sep 15, 2016

View reviewed changes

alisd23 force-pushed the tooltip-delay-prop branch 2 times, most recently from ddc1164 to dd9fd68 Compare September 16, 2016 12:46

alisd23 force-pushed the tooltip-delay-prop branch from dd9fd68 to be8a2b4 Compare September 17, 2016 15:37

test: add tests for delay prop object/number

968596c

alisd23 force-pushed the tooltip-delay-prop branch from be8a2b4 to 968596c Compare September 17, 2016 15:39

eddywashere requested changes Sep 17, 2016

View reviewed changes

TheSharpieOne reviewed Sep 17, 2016

View reviewed changes

feat: allow partial delay object and add tests

1923fe6

alisd23 force-pushed the tooltip-delay-prop branch from b8394ec to 1923fe6 Compare September 17, 2016 21:22

TheSharpieOne approved these changes Sep 17, 2016

View reviewed changes

eddywashere approved these changes Sep 18, 2016

View reviewed changes

eddywashere merged commit b18fb74 into reactstrap:master Sep 18, 2016

eddywashere mentioned this pull request Sep 18, 2016

chore(release): adding 3.2.1 #148

Merged

eddywashere mentioned this pull request Sep 28, 2016

Add Alert Component #162

Merged

Uh oh!

Conversation

alisd23 commented Sep 15, 2016

Uh oh!

TheSharpieOne Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSharpieOne Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSharpieOne commented Sep 16, 2016

Uh oh!

alisd23 commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheSharpieOne commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alisd23 commented Sep 17, 2016

Uh oh!

eddywashere commented Sep 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddywashere commented Sep 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSharpieOne left a comment

Choose a reason for hiding this comment

Uh oh!

eddywashere left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheSharpieOne Sep 15, 2016 •

edited

Loading

TheSharpieOne Sep 15, 2016 •

edited

Loading

alisd23 commented Sep 16, 2016 •

edited

Loading

TheSharpieOne commented Sep 16, 2016 •

edited

Loading