Allow same day input when `minimumNights={0}` by timhwang21 · Pull Request #555 · react-dates/react-dates

timhwang21 · 2017-06-12T01:15:26Z

Fixes #353.

ljharb

Thanks, this looks great overall!

ljharb · 2017-06-12T02:11:33Z

  keepOpenOnDateSelect: PropTypes.bool,
  reopenPickerOnClearDates: PropTypes.bool,
  withFullScreenPortal: PropTypes.bool,
+  minimumNights: PropTypes.number,


this should probably be a non-negative integer (nonNegativeInteger from airbnb-prop-types), not just a number

ljharb · 2017-06-12T02:12:14Z

+        it('calls props.onDatesChange with provided end date', () => {
+          const onDatesChangeStub = sinon.stub();
+          const wrapper =
+            shallow(<DateRangePickerInputController onDatesChange={onDatesChangeStub} />);


please don't add line breaks after assignments (same throughout, even if that was already in the file); if this is too long, let's indent like so:

const wrapper = shallow(( <DateRangePickerInputController onDatesChange={onDatesChangeStub} /> ));

+1, on a related note, wondering if there are plans to add anything like Prettier to the code base?

Definitely not; if there's anything eslint doesn't do, let's make eslint better. I'm not yet aware of anything prettier is theoretically capable of that eslint theoretically isn't :-)

ljharb · 2017-06-12T02:13:07Z

+          wrapper.instance().onEndDateChange(validFutureDateString);
+          expect(onDatesChangeStub.callCount).to.equal(1);
+
+          const onDatesChangeArgs = onDatesChangeStub.getCall(0).args[0];


const [{ startDate, endDate }] = onDatesChangeStub.getCall(0).args;

ljharb · 2017-06-12T02:31:09Z

+                onDatesChange={onDatesChangeStub}
+                endDate={endDate}
+                minimumNights={1}
+              />);


similarly, since the opening ( ends a line, the closing ) should start a line.

timhwang21 · 2017-06-12T02:54:49Z

-          const wrapper =
-            shallow(<DateRangePickerInputController onFocusChange={onFocusChangeStub} />);
+          const wrapper = shallow(
+            <DateRangePickerInputController onFocusChange={onFocusChangeStub} />,


@ljharb putting a trailing comma seems like the more consistent pattern in the codebase vs. double parens, does this look right to you? Thanks!

yes, either one's totally fine :-)

unfortunately comma-dangle for functions doesn't have an exception for "1-arg jsx", which is the one case it looks weird.

majapw

Looking good! One quick Q

majapw · 2017-06-12T22:51:33Z

    const isEndDateValid = endDate && !isOutsideRange(endDate) &&
-      !isInclusivelyBeforeDay(endDate, startDate);
+      (!isInclusivelyBeforeDay(endDate, startDate) ||
+      (minimumNights === 0 && isSameDay(endDate, startDate)));


This is a bit of a nit, but could we change this to:

( !isInclusivelyBeforeDay(endDate, startDate) || (minimumNights === 0 && isSameDay(endDate, startDate)) );

Also, I feel like it's a bit weird that we don't check for minimum nights requirements in other scenarios. Maybe this should just be:

isInclusivelyAfterDay(endDate, startDate + minimumNights)

Would that work?

+1 to the first part, and ah yeah, I see how the Inclusively functions work now, should have taken a closer look -- !isInclusivelyBeforeDay() is equivalent to isAfterDay() so yeah, makes sense to me. Though we'd have to do startDate.clone().add(minimumNights, 'days'). Do you think it's worth the clone?

Thanks!

Hey! Wondering if there's anything else I can do WRT this PR? Thanks!

I think this is worth the clone in this scenario. Let's change it to be isInclusivelyAfterDay(endDate, startDate + minimumNights).

Fixes react-dates#353.

* Proptypes.number => nonNegativeInteger * Fix no linebreak after assignment * Fix training parentheses not having linebreak when leading parentheses does

coveralls · 2017-06-30T01:57:48Z

Coverage remained the same at 86.13% when pulling f2b572c on timhwang21:timhwang21/353-manual-input-breaks-for-same-day into c96a237 on airbnb:master.

ljharb

LGTM; deferring to @majapw

majapw

This seems legit to me. I'd prefer the isInclusivelyAfterDay(endDate, startDate + minimumNights) call for the clarity of code, despite the cloning required. It doesn't get called too much so i think it should be okay!

Used `isBeforeDay` instead of `isInclusivelyAfterDay` because to clone, a null check is needed for `startDate`. And when doing the same, reversed check in `onStartDateChange()` the double negative `!` looks bad.

…e-day

timhwang21 · 2017-06-30T22:04:09Z

Sounds good, made the change (though I used isBeforeDay to make the boolean logic less ugly)

coveralls · 2017-06-30T22:14:15Z

Coverage remained the same at 86.266% when pulling 312216b on timhwang21:timhwang21/353-manual-input-breaks-for-same-day into c384dc1 on airbnb:master.

coveralls · 2017-06-30T22:18:25Z

Coverage remained the same at 86.266% when pulling 312216b on timhwang21:timhwang21/353-manual-input-breaks-for-same-day into c384dc1 on airbnb:master.

majapw

LGTM!

ljharb requested changes Jun 12, 2017

View reviewed changes

ljharb added semver-minor: new stuff Any feature or API addition. semver-patch: fixes/refactors/etc Anything that's not major or minor. labels Jun 12, 2017

timhwang21 commented Jun 12, 2017

View reviewed changes

majapw requested changes Jun 12, 2017

View reviewed changes

timhwang21 added 2 commits June 29, 2017 21:45

Allow same day input when minimumNights={0}

d8aa65a

Fixes react-dates#353.

Code style fixes

f2b572c

* Proptypes.number => nonNegativeInteger * Fix no linebreak after assignment * Fix training parentheses not having linebreak when leading parentheses does

timhwang21 force-pushed the timhwang21/353-manual-input-breaks-for-same-day branch from c03e69d to f2b572c Compare June 30, 2017 01:46

timhwang21 mentioned this pull request Jun 30, 2017

Allow clear dates to clear partially entered dates #554

Closed

ljharb previously approved these changes Jun 30, 2017

View reviewed changes

majapw previously approved these changes Jun 30, 2017

View reviewed changes

Refactor away minimumNights check

be4ef1e

Used `isBeforeDay` instead of `isInclusivelyAfterDay` because to clone, a null check is needed for `startDate`. And when doing the same, reversed check in `onStartDateChange()` the double negative `!` looks bad.

timhwang21 dismissed stale reviews from majapw and ljharb via be4ef1e June 30, 2017 22:02

Merge branch 'master' into timhwang21/353-manual-input-breaks-for-sam…

312216b

…e-day

majapw approved these changes Jul 14, 2017

View reviewed changes

majapw merged commit 19a10f7 into react-dates:master Jul 14, 2017

timhwang21 deleted the timhwang21/353-manual-input-breaks-for-same-day branch July 14, 2017 22:32

Uh oh!

Conversation

timhwang21 commented Jun 12, 2017

Uh oh!

ljharb left a comment

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!

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!

majapw left a comment

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!

coveralls commented Jun 30, 2017

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

majapw left a comment

Choose a reason for hiding this comment

Uh oh!

timhwang21 commented Jun 30, 2017

Uh oh!

coveralls commented Jun 30, 2017

Uh oh!

coveralls commented Jun 30, 2017

Uh oh!

majapw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants