[Feature] - Add onClose callback #397

rafacianci · 2017-03-23T14:14:25Z

This PR is related to #264

We are having some troubles when the user close the picker on the <DateRangePicker /> component, because it calls the onDatesChange and the onFocusChange at the same time, and when the onFocusChange is called without the focus there wasn't time to reset the state.

RaonyMarcondes

🏆

coveralls · 2017-03-23T14:37:51Z

Coverage increased (+0.1%) to 87.468% when pulling 36a8af6 on viniciusdacal:issue-264 into b90ee78 on airbnb:master.

majapw

This seems reasonable to me. Can you added the same functionality to the SingleDatePicker, update the README, and squash your commits? I'll merge it in after that. :)

viniciusdacal · 2017-04-01T20:06:38Z

test/components/DateRangePickerInputController_spec.jsx

@@ -90,6 +90,25 @@ describe('DateRangePickerInputController', () => {
      wrapper.instance().onClearFocus();
      expect(onFocusChangeStub.calledWith(null)).to.equal(true);
    });
+
+    it('calls props.onFocusChange with startDate and endDate args', () => {


I think here it was supposed to be props.onClose

coveralls · 2017-04-04T15:21:45Z

Coverage increased (+0.3%) to 83.597% when pulling ad9be6a on viniciusdacal:issue-264 into 31d2764 on airbnb:master.

viniciusdacal · 2017-04-04T15:59:01Z

README.md

@@ -171,6 +172,7 @@ navPrev: PropTypes.node,
 navNext: PropTypes.node,
 onPrevMonthClick: PropTypes.func,
 onNextMonthClick: PropTypes.func,
+onClose: ({ startDate, endDate }) => this.setState({ startDate, endDate }) // PropTypes.func,


shouldn't it be onclose: PropTypes.func instead?

I put the function to let the user know how to use the callback... I didn't what would be the better way to show it. Any ideias?

English text in the documentation. propTypes are not for a human, they're for React to issue warnings when it doesn't match.

I think we could set examples above, after line 77

viniciusdacal · 2017-04-04T16:00:10Z

README.md

@@ -111,6 +111,7 @@ navPrev: PropTypes.node,
 navNext: PropTypes.node,
 onPrevMonthClick: PropTypes.func,
 onNextMonthClick: PropTypes.func,
+onClose: ({ startDate, endDate }) => this.setState({ startDate, endDate }) // PropTypes.func,


do we need the comment here? Same case line 175

viniciusdacal · 2017-04-04T16:02:18Z

src/components/DateRangePicker.jsx

@@ -370,6 +375,8 @@ export default class DateRangePicker extends React.Component {
      reopenPickerOnClearDates,
      keepOpenOnDateSelect,
      onDatesChange,
+      onFocusChange,


Are we using onFocusChange in this scope?

ljharb

It seems like you have a defaultProp inside propTypes by mistake?

viniciusdacal · 2017-04-04T20:02:45Z

README.md

@@ -171,6 +172,7 @@ navPrev: PropTypes.node,
 navNext: PropTypes.node,
 onPrevMonthClick: PropTypes.func,
 onNextMonthClick: PropTypes.func,
+onClose: ({ startDate, endDate }) => this.setState({ startDate, endDate }) // PropTypes.func,


And this one, would go after line 142

viniciusdacal

LGTM 🏆

coveralls · 2017-04-04T20:52:59Z

Coverage increased (+0.3%) to 83.597% when pulling 8a13122 on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

rafacianci · 2017-04-04T20:55:29Z

@ljharb and @majapw I updated the Pull Request, can you review it again? please...
thanks.

coveralls · 2017-04-04T20:56:53Z

Coverage increased (+0.3%) to 83.597% when pulling 8a13122 on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

ljharb · 2017-04-04T21:23:19Z

src/components/DateRangePickerInputController.jsx

@@ -123,7 +129,10 @@ export default class DateRangePickerInputController extends React.Component {
      !isInclusivelyBeforeDay(endDate, startDate);
    if (isEndDateValid) {
      onDatesChange({ startDate, endDate });
-      if (!keepOpenOnDateSelect) onFocusChange(null);
+      if (!keepOpenOnDateSelect) {
+        onFocusChange(null);


Maybe this should call this.onClearFocus instead? (To avoid repeating this pair of statements)

ljharb · 2017-04-04T21:24:05Z

src/components/DayPickerRangeController.jsx

@@ -147,7 +149,10 @@ export default class DayPickerRangeController extends React.Component {
        this.props.onFocusChange(START_DATE);
      } else if (isInclusivelyAfterDay(day, firstAllowedEndDate)) {
        endDate = day;
-        if (!keepOpenOnDateSelect) this.props.onFocusChange(null);
+        if (!keepOpenOnDateSelect) {
+          this.props.onFocusChange(null);


Let's destructure out these prop funcs so their this isn't the props object

rafacianci · 2017-04-05T11:53:31Z

done @ljharb.. can you review it?

coveralls · 2017-04-05T12:04:30Z

Coverage increased (+0.3%) to 83.55% when pulling e4443d8 on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

ljharb · 2017-04-05T17:46:55Z

src/components/DayPickerRangeController.jsx

-        if (!keepOpenOnDateSelect) this.props.onFocusChange(null);
+        if (!keepOpenOnDateSelect) {
+          onFocusChange(null);
+          this.props.onClose({ startDate, endDate });


Ideally this would be destructured out too

ahh ok.. I didnt because it was using just so I thought it should be better to use the props.. But I removing.. ;)

done. @ljharb :D

Variables are free; passing the props object to user code is not :-)

nice.. :)
can you approve it?
and.. can I merge and squash?

You can always rebase down to an atomic set of commits; please do so frequently :-)

done @ljharb :)

coveralls · 2017-04-05T18:07:27Z

Coverage increased (+0.3%) to 83.55% when pulling 06a40aa on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

coveralls · 2017-04-05T18:07:27Z

Coverage increased (+0.3%) to 83.55% when pulling 06a40aa on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

coveralls · 2017-04-06T13:52:19Z

Coverage increased (+0.3%) to 83.55% when pulling f84442e on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

coveralls · 2017-04-06T13:52:19Z

Coverage increased (+0.3%) to 83.55% when pulling f84442e on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

coveralls · 2017-04-06T14:58:55Z

Coverage increased (+0.3%) to 83.55% when pulling e38362a on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

coveralls · 2017-04-06T14:58:55Z

Coverage increased (+0.3%) to 83.55% when pulling e38362a on viniciusdacal:issue-264 into 5ba9930 on airbnb:master.

ljharb

Seems good; deferring to @majapw

rafacianci · 2017-04-12T13:09:00Z

@majapw can you review it?

majapw

Looks great! Let's get her merged! :)

coveralls · 2017-04-12T18:14:57Z

Coverage increased (+0.2%) to 83.909% when pulling 527fa56 on viniciusdacal:issue-264 into f9370fb on airbnb:master.

coveralls · 2017-04-12T18:14:57Z

Coverage increased (+0.2%) to 83.909% when pulling 527fa56 on viniciusdacal:issue-264 into f9370fb on airbnb:master.

rafacianci · 2017-04-12T18:20:50Z

\o// thanks.. ☺️

majapw · 2017-04-12T18:22:00Z

@ljharb The new travis tests are failing in a weird way. Specifically there seems to be an error with installing enzyme when testing with React 15... Can you take a look?

coveralls · 2017-04-12T18:35:18Z

Coverage increased (+0.2%) to 83.909% when pulling d7610b5 on viniciusdacal:issue-264 into f9370fb on airbnb:master.

ljharb · 2017-04-12T21:18:02Z

It's because React 15.5, due to new warnings, is a breaking change for us and for enzyme. The quick fix is to lock React down to < 15.5; pending the changes required to run the tests on both 15.4 and 15.5+.

majapw · 2017-04-12T21:24:58Z

@ljharb Enzyme has a fix, I just need to add react-test-renderer to my deps according to @lelandrichardson, which I will do. :)

majapw · 2017-04-12T21:34:47Z

PR open here - #442

coveralls · 2017-04-12T22:33:07Z

Coverage increased (+0.2%) to 83.909% when pulling 4962f24 on viniciusdacal:issue-264 into 45eee40 on airbnb:master.

lukedesu · 2017-04-13T03:29:51Z

just pull the latest code and got a warning message like:
Warning: Unknown prop onClose on tag. Remove this prop from the element.

ljharb · 2017-04-13T09:36:53Z

@senseluo filed as #444; fix incoming.

haunguyen90 · 2017-04-13T10:06:08Z

great.... waiting for the update

RaonyMarcondes approved these changes Mar 23, 2017

View reviewed changes

majapw requested changes Mar 30, 2017

View reviewed changes

viniciusdacal reviewed Apr 1, 2017

View reviewed changes

viniciusdacal reviewed Apr 4, 2017

View reviewed changes

ljharb requested changes Apr 4, 2017

View reviewed changes

viniciusdacal reviewed Apr 4, 2017

View reviewed changes

viniciusdacal approved these changes Apr 4, 2017

View reviewed changes

ljharb reviewed Apr 4, 2017

View reviewed changes

ljharb reviewed Apr 5, 2017

View reviewed changes

rafacianci force-pushed the issue-264 branch from f84442e to e38362a Compare April 6, 2017 14:43

ljharb approved these changes Apr 6, 2017

View reviewed changes

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

majapw approved these changes Apr 12, 2017

View reviewed changes

majapw force-pushed the issue-264 branch from e38362a to 527fa56 Compare April 12, 2017 18:04

majapw force-pushed the issue-264 branch from 527fa56 to d7610b5 Compare April 12, 2017 18:16

implement onClose

4962f24

majapw force-pushed the issue-264 branch from d7610b5 to 4962f24 Compare April 12, 2017 22:21

majapw merged commit 98232c3 into react-dates:master Apr 12, 2017

ljharb mentioned this pull request Apr 13, 2017

React DOM Unknown Prop Warning on implementation of onClose #444

Closed

[Feature] - Add onClose callback #397

[Feature] - Add onClose callback #397

Conversation

rafacianci commented Mar 23, 2017

RaonyMarcondes left a comment

Choose a reason for hiding this comment

coveralls commented Mar 23, 2017

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 4, 2017 • 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

viniciusdacal Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciusdacal left a comment

Choose a reason for hiding this comment

coveralls commented Apr 4, 2017 • edited Loading

rafacianci commented Apr 4, 2017

coveralls commented Apr 4, 2017 • 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

rafacianci commented Apr 5, 2017

coveralls commented Apr 5, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 5, 2017

coveralls commented Apr 5, 2017

coveralls commented Apr 6, 2017

coveralls commented Apr 6, 2017

coveralls commented Apr 6, 2017

coveralls commented Apr 6, 2017 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

rafacianci commented Apr 12, 2017

majapw left a comment • edited Loading

Choose a reason for hiding this comment

coveralls commented Apr 12, 2017

coveralls commented Apr 12, 2017

rafacianci commented Apr 12, 2017

majapw commented Apr 12, 2017

coveralls commented Apr 12, 2017

ljharb commented Apr 12, 2017

majapw commented Apr 12, 2017

majapw commented Apr 12, 2017

coveralls commented Apr 12, 2017 • edited Loading

lukedesu commented Apr 13, 2017

ljharb commented Apr 13, 2017

haunguyen90 commented Apr 13, 2017

coveralls commented Apr 4, 2017 •

edited

Loading

viniciusdacal Apr 4, 2017 •

edited

Loading

coveralls commented Apr 4, 2017 •

edited

Loading

coveralls commented Apr 4, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 6, 2017 •

edited

Loading

majapw left a comment •

edited

Loading

coveralls commented Apr 12, 2017 •

edited

Loading