Added OutsideClickHandler component with onOutsideClick callback #666

mattRoyten · 2017-08-07T21:27:16Z

Corrected onOutsideClick functionality missing from the DayPickerSingleDateController component. This prop was within the defaultProps and now is properly applied.

coveralls · 2017-08-07T21:38:08Z

Coverage remained the same at 86.243% when pulling 06bc627 on mattRoyten:outsideClick into d32e9b3 on airbnb:master.

ljharb · 2017-08-07T22:40:00Z

src/components/DayPickerSingleDateController.jsx

-        isRTL={isRTL}
-      />
+      <OutsideClickHandler
+        onOutsideClick={onOutsideClick}


if onOutsideClick is omitted, should the DayPicker still be wrapped?

I do not see any negative impact if onOutsideClick is omitted. Are you thinking to conditionally wrap the DayPicker component depending on the onOutsideClick value?

Yes, that way if people don't want outside click behavior, they wouldn't incur any overhead.

coveralls · 2017-08-08T18:16:01Z

Coverage increased (+0.02%) to 86.263% when pulling 601dd36 on mattRoyten:outsideClick into d32e9b3 on airbnb:master.

mattRoyten · 2017-08-09T11:44:41Z

@ljharb - Updated PR with suggestion.

ljharb

Thanks - I was thinking more, not wrapping it at all rather than wrapping it in a div - you could store the DayPicker element in a variable, and then repeat that in both if/else code paths.

ljharb · 2017-08-09T15:42:53Z

src/components/DayPickerSingleDateController.jsx

    } = this.props;

    const { currentMonth, visibleDays } = this.state;

+    const DayPickerWrapper = (onOutsideClick) ? OutsideClickHandler : 'div';


Let's remove these unnecessary parents on the ternary conditionals.

ljharb · 2017-08-09T15:43:17Z

src/components/DayPickerSingleDateController.jsx

    } = this.props;

    const { currentMonth, visibleDays } = this.state;

+    const DayPickerWrapper = (onOutsideClick) ? OutsideClickHandler : 'div';
+    const wrapperProps = (onOutsideClick) ? { onOutsideClick } : {};


This shouldn't be needed; onOutsideClick={null} on a DOM element is a no-op.

mattRoyten · 2017-08-09T18:24:33Z

@ljharb - Thanks for the feedback, updated PR.

coveralls · 2017-08-09T18:34:42Z

Coverage decreased (-9.8%) to 76.633% when pulling 065d567 on mattRoyten:outsideClick into d12633f on airbnb:master.

coveralls · 2017-08-09T18:36:45Z

Coverage decreased (-9.8%) to 76.633% when pulling 065d567 on mattRoyten:outsideClick into d12633f on airbnb:master.

coveralls · 2017-08-09T19:30:28Z

Coverage increased (+0.01%) to 86.483% when pulling 2e6e9b3 on mattRoyten:outsideClick into d12633f on airbnb:master.

majapw · 2017-08-09T20:12:17Z

I'm interested in why the refactor failed. I too think I would prefer what @ljharb suggested and don't know why that would cause tests to fail.

mattRoyten · 2017-08-10T02:41:51Z

@majapw - all of the DayPickerSingleDateController tests were getting the following error:
Invariant Violation: Unexpected node: [object Object]

ljharb · 2017-08-10T03:24:25Z

@mattRoyten You had an error in your code, in the unwrapped case, you were returning an object (ie, { dayPickerComponent } instead of directly returning dayPickerComponent.

Could you rebase on the command line so as to remove all merge commits, and also remove the last revert commit, and fix that error?

mattRoyten · 2017-08-10T13:55:42Z

@ljharb - got it. Updated.

coveralls · 2017-08-10T14:06:20Z

Coverage decreased (-0.04%) to 86.43% when pulling 24662b8 on mattRoyten:outsideClick into 9685cc9 on airbnb:master.

ljharb

LGTM, but I'm deferring to @majapw

ljharb · 2017-08-10T16:49:25Z

src/components/DayPickerSingleDateController.jsx

-        isRTL={isRTL}
-      />
-    );
+    const dayPickerComponent = (<DayPicker


one last nit: the ( should end the line, and the ); should begin one - iow:

const foo = ( <div> </div> );

coveralls · 2017-08-10T17:38:04Z

Coverage decreased (-0.04%) to 86.43% when pulling e5855a6 on mattRoyten:outsideClick into 9685cc9 on airbnb:master.

ljharb

Looks like we need some test coverage of this - a single shallow-render should do it :-)

majapw

Let's add a test! :)

(one that checks for the presence of the OutsideClickHandler when that's passed in and for its absence when it's not should be sufficient)

majapw · 2017-08-10T23:03:36Z

src/components/DayPickerSingleDateController.jsx

@@ -599,6 +601,18 @@ export default class DayPickerSingleDateController extends React.Component {
        isRTL={isRTL}
      />
    );
+
+    if (onOutsideClick) {


Oh this looks so much better

mattRoyten · 2017-08-11T02:49:36Z

@majapw - tests added!

coveralls · 2017-08-11T02:58:51Z

Coverage increased (+0.03%) to 86.502% when pulling 7b59f4f on mattRoyten:outsideClick into 9685cc9 on airbnb:master.

ljharb

Looks great!

ljharb · 2017-08-11T03:05:58Z

test/components/DayPickerSingleDateController_spec.jsx

+          onOutsideClick={() => null}
+        />,
+      );
+      expect(wrapper.find(OutsideClickHandler).length).to.equal(1);


expect(wrapper.find(OutsideClickHandler).to.have.lengthOf(1) has a nicer failure message; nbd tho :-)

sure, want me to change this as well?

Sure, that'd be great :-D

majapw

LGTM!

coveralls · 2017-08-14T19:01:40Z

Coverage increased (+0.03%) to 86.502% when pulling 8a328fb on mattRoyten:outsideClick into 9685cc9 on airbnb:master.

ljharb reviewed Aug 7, 2017

View reviewed changes

ljharb requested changes Aug 9, 2017

View reviewed changes

return component instead of object for dayPickerComponent

24662b8

mattRoyten force-pushed the outsideClick branch from 2e6e9b3 to 24662b8 Compare August 10, 2017 13:53

ljharb previously approved these changes Aug 10, 2017

View reviewed changes

ljharb requested a review from majapw August 10, 2017 16:50

ljharb added the semver-minor: new stuff Any feature or API addition. label Aug 10, 2017

minor format update

e5855a6

mattRoyten dismissed ljharb’s stale review via e5855a6 August 10, 2017 17:26

ljharb requested changes Aug 10, 2017

View reviewed changes

majapw requested changes Aug 10, 2017

View reviewed changes

added shallow render tests for OutsideClickHandler

7b59f4f

ljharb previously approved these changes Aug 11, 2017

View reviewed changes

majapw previously approved these changes Aug 11, 2017

View reviewed changes

updated test from feedback

8a328fb

mattRoyten dismissed stale reviews from majapw and ljharb via 8a328fb August 14, 2017 18:50

ljharb approved these changes Aug 14, 2017

View reviewed changes

ljharb merged commit 57a0f37 into react-dates:master Aug 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OutsideClickHandler component with onOutsideClick callback #666

Added OutsideClickHandler component with onOutsideClick callback #666

mattRoyten commented Aug 7, 2017

coveralls commented Aug 7, 2017

ljharb Aug 7, 2017

mattRoyten Aug 8, 2017

ljharb Aug 8, 2017

coveralls commented Aug 8, 2017

mattRoyten commented Aug 9, 2017

ljharb left a comment

ljharb Aug 9, 2017

ljharb Aug 9, 2017

mattRoyten commented Aug 9, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

majapw commented Aug 9, 2017

mattRoyten commented Aug 10, 2017

ljharb commented Aug 10, 2017

mattRoyten commented Aug 10, 2017

coveralls commented Aug 10, 2017

ljharb left a comment

ljharb Aug 10, 2017

coveralls commented Aug 10, 2017

ljharb left a comment

majapw left a comment

majapw Aug 10, 2017

mattRoyten commented Aug 11, 2017

coveralls commented Aug 11, 2017

ljharb left a comment

ljharb Aug 11, 2017

mattRoyten Aug 14, 2017

ljharb Aug 14, 2017

mattRoyten Aug 14, 2017

majapw left a comment

coveralls commented Aug 14, 2017

Added OutsideClickHandler component with onOutsideClick callback #666

Added OutsideClickHandler component with onOutsideClick callback #666

Conversation

mattRoyten commented Aug 7, 2017

coveralls commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 8, 2017

mattRoyten commented Aug 9, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattRoyten commented Aug 9, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

coveralls commented Aug 9, 2017

majapw commented Aug 9, 2017

mattRoyten commented Aug 10, 2017

ljharb commented Aug 10, 2017

mattRoyten commented Aug 10, 2017

coveralls commented Aug 10, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 10, 2017

ljharb left a comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattRoyten commented Aug 11, 2017

coveralls commented Aug 11, 2017

ljharb 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

majapw left a comment

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017