Allow same day input when `minimumNights={0}` #555

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

Fixes #353.

ljharb

Thanks, this looks great overall!

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

src/components/DateRangePickerInputController.jsx

@@ -41,6 +42,7 @@ const propTypes = forbidExtraProps({
  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

test/components/DateRangePickerInputController_spec.jsx

+        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

test/components/DateRangePickerInputController_spec.jsx

+          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

test/components/DateRangePickerInputController_spec.jsx

+                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

test/components/DateRangePickerInputController_spec.jsx

-          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

src/components/DateRangePickerInputController.jsx

      keepOpenOnDateSelect,
      onDatesChange,
    } = this.props;

    const endDate = toMomentObject(endDateString, this.getDisplayFormat());

    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow same day input when `minimumNights={0}` #555

Allow same day input when `minimumNights={0}` #555

timhwang21 commented Jun 12, 2017

ljharb left a comment

ljharb Jun 12, 2017

ljharb Jun 12, 2017

timhwang21 Jun 12, 2017

ljharb Jun 12, 2017

ljharb Jun 12, 2017

ljharb Jun 12, 2017

timhwang21 Jun 12, 2017

ljharb Jun 12, 2017

majapw left a comment

majapw Jun 12, 2017

timhwang21 Jun 13, 2017

timhwang21 Jun 18, 2017

majapw Jun 30, 2017

coveralls commented Jun 30, 2017

ljharb left a comment

majapw left a comment

timhwang21 commented Jun 30, 2017

coveralls commented Jun 30, 2017

coveralls commented Jun 30, 2017

majapw left a comment

Allow same day input when minimumNights={0} #555

Allow same day input when minimumNights={0} #555

Conversation

timhwang21 commented Jun 12, 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

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

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 Jun 30, 2017

ljharb left a comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

timhwang21 commented Jun 30, 2017

coveralls commented Jun 30, 2017

coveralls commented Jun 30, 2017

majapw left a comment

Choose a reason for hiding this comment

Allow same day input when `minimumNights={0}` #555

Allow same day input when `minimumNights={0}` #555