minDate and maxDate to block navigation #1311

pedroabreu · 2018-08-14T22:20:11Z

Adds feature to block month navigation depending on a minDate/maxDate prop.

Resolves #339

coveralls · 2018-08-14T23:43:34Z

Coverage increased (+0.03%) to 85.104% when pulling 9767c36 on pedroabreu:feat/disable-navigation into 9426234 on airbnb:master.

ljharb · 2018-08-15T01:55:36Z

src/components/DayPickerNavigation.jsx

@@ -33,6 +35,8 @@ const propTypes = forbidExtraProps({
 });

 const defaultProps = {
+  hasPrev: true,


boolean props should default to false, so that absence and falsiness are the same. Can the naming of these be inverted?

Agreed! Let's call this disablePrev and disableNext instead

Fair point. I've come up with isPrevMonthBlocked/isNextMonthBlocked, to try and keep it consistent with isDayBlocked prop.

ljharb · 2018-08-15T01:56:58Z

src/components/DayPickerRangeController.jsx

+      enableOutsideDays,
+    } = this.props;
+
+    const isMinDateVisible = isDayVisible(minDate, visibleMonth, numberOfMonths, enableOutsideDays);


If minDate is falsy, it seems like there’s no point in calling isDayVisible?

Yep, it will return false in that case.

Happy to change it to make it more readable. I guess it's a good thing not to rely on the method accounting for that scenario (non-moment date). Make it less "magical"

ljharb · 2018-08-15T01:57:02Z

src/components/DayPickerRangeController.jsx

+    } = this.props;
+
+    const isMinDateVisible = isDayVisible(minDate, visibleMonth, numberOfMonths, enableOutsideDays);
+    const isMaxDateVisible = isDayVisible(maxDate, visibleMonth, numberOfMonths, enableOutsideDays);


ljharb · 2018-08-15T01:57:52Z

src/utils/isDayVisible.js

 import isBeforeDay from './isBeforeDay';
 import isAfterDay from './isAfterDay';

 export default function isDayVisible(day, month, numberOfMonths, enableOutsideDays) {
+  if (!moment.isMoment(day)) return false;


This is a good change, even if my above comments are addressed (either this function should throw or return false on a non-moment object)

Followed the same pattern as other utils that do an early return if non-moment object, not sure if it was intended not to have a check but the tests don't break

majapw

I'm super hype on this idea. Thank you for contributing!

One thing I was thinking about is that I'd love to align this somehow with the concept of the blocked-outside-range modifier. Specifically, I'd like to eventually replace isOutsideRange with minDate and maxDate in a follow-up breaking change. However, that means that I don't think disabling the month navigation should be intrinsically tied to these props. Maybe we could have:

maxInRangeDate
minInRangeDate
disableMonthNavOnOutsideRange

(it could be less verbose, but that's the general gist of what I am thinking)

majapw · 2018-08-15T17:25:20Z

src/components/DayPicker.jsx

@@ -73,6 +73,8 @@ const propTypes = forbidExtraProps({
  horizontalMonthPadding: nonNegativeInteger,

  // navigation props
+  hasPrev: PropTypes.bool,


Let's call this disablePrev and disableNext to address Jordan's comments.

majapw · 2018-08-15T17:25:42Z

src/components/DayPickerNavigation.jsx

@@ -33,6 +35,8 @@ const propTypes = forbidExtraProps({
 });

 const defaultProps = {
+  hasPrev: true,


Agreed! Let's call this disablePrev and disableNext instead

majapw · 2018-08-15T17:26:55Z

src/components/DayPickerNavigation.jsx

@@ -138,8 +147,9 @@ function DayPickerNavigation({
            ]),
          )}
          aria-label={phrases.jumpToPrevMonth}
-          onClick={onPrevMonthClick}
+          onClick={hasPrev ? onPrevMonthClick : () => {}}


let's have this be undefined if it's disabled, so something like hasPrev ? onPrevMonthClick : undefined instead

majapw · 2018-08-15T17:27:22Z

src/components/DayPickerNavigation.jsx

          onKeyUp={(e) => {
+            if (!hasPrev) return;


Same here. let's just not define this function if hasPrev is false.

majapw · 2018-08-15T17:27:37Z

src/components/DayPickerNavigation.jsx

@@ -177,8 +188,9 @@ function DayPickerNavigation({
          ]),
        )}
        aria-label={phrases.jumpToNextMonth}
-        onClick={onNextMonthClick}
+        onClick={hasNext ? onNextMonthClick : () => {}}


Same comments apply here about using undefined instead of a thunk

Curious: I guess there's an advantage by using undefined instead of a thunk ?

Setting it to undefined is akin to not having that prop set at all, which means no unnecessary click handler. Otherwise you're adding a click handlers to things that don't need them, only to call an empty function upon invocation.

majapw · 2018-08-15T17:27:48Z

src/components/DayPickerNavigation.jsx

        onKeyUp={(e) => {
+          if (!hasNext) return;


Let's not define this when we're disabling the button.

src/components/DayPickerNavigation.jsx

majapw · 2018-08-15T17:30:16Z

src/components/DayPickerRangeController.jsx

@@ -747,6 +752,23 @@ export default class DayPickerRangeController extends React.Component {
    });
  }

+  getNavigationStatus(visibleMonth) {


Hmm, I'm not sure that this name is clear. I'd almost rather have two methods, one that asks if we should disable prev month navigation and one that asks about next month navigation.

I think it should be kept as one method, as the only difference between them would be the arguments (date, visibleMonth). Maybe shouldDisableMonthNavigation ?

pedroabreu · 2018-08-15T20:24:22Z

@majapw I'm not that familiar with all features of the component but is isOutsideRange used to detect if a range is bigger than the one specified ?

I noticed that isOutsideRange is a function, so is there any range specified besides minimumNights ?

I can see a use case (not sure if it's the one you're mentioning) where I specify that I can only select a maxNumberOfNights of 4 months, so the calendar would block navigation past/before 4 months from the startDate.

Can you expand a bit on your idea ? I can try to have a look (perhaps not for this PR)

Also, I think I addresed all the comments (cc @ljharb)

LGTM, deferring to maja and diane

pedroabreu · 2018-09-04T11:04:22Z

@majapw any other thoughts on this ?

src/components/DayPickerRangeController.jsx

saadulde45 · 2018-10-12T11:20:30Z

Any update on this? This seems like a good feature to have, if there are no blockers can it please be merged?

pedroabreu · 2018-10-16T06:22:25Z

Anything missing ? cc @majapw @ljharb @backwardok

ljharb · 2018-10-19T20:32:21Z

Please do rebase it, instead of clicking "update branch" which produces a merge commit.

pedroabreu · 2018-11-17T01:12:15Z

Anything missing ? cc @majapw @ljharb @backwardok

backwardok

Sorry for the delay! I'm good after the changes I suggested but I defer stamping to @majapw since she's more familiar with the codebase than I am

src/components/DayPickerNavigation.jsx

src/components/DayPickerRangeController.jsx

Co-Authored-By: pedroabreu <pmig.abreu@gmail.com>

elinskytap · 2018-11-28T00:03:51Z

@majapw do you have time to review this? Really looking forward to seeing this to be released. Thanks!!

avivash · 2019-03-12T23:03:13Z

I see these props were only added to DayPickerRangeController. Is there any way to use them with the DateRangePicker component? CC: @pedroabreu @ljharb @backwardok

pedroabreu · 2019-03-12T23:30:59Z

Seems to be as easy as adding minDate and maxDate to the DateRangePicker component. I can have a look with more time tomorrow

avivash · 2019-03-12T23:33:04Z

@pedroabreu thanks for the quick reply. When I try adding them to DateRangePicker, I get an error, as it doesn't recognize the props

pedroabreu · 2019-03-13T07:48:31Z

Sorry, I mean add them as props to the component and pass it down to the controller.

…

On Tue, Mar 12, 2019 at 23:33 Andrew Vivash ***@***.***> wrote: @pedroabreu <https://github.com/pedroabreu> thanks for the quick reply. When I try adding them to DateRangePicker, I get an error, as it doesn't recognize the props [image: image] <https://user-images.githubusercontent.com/1179291/54243137-7ce31580-44e4-11e9-85cb-fa045058f2fe.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1311 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABI7iEZrWIrTaf6pqFA4drpaMZUVJQNpks5vWDk1gaJpZM4V9RGc> .

avivash · 2019-03-13T17:07:26Z

Sorry, I mean add them as props to the component and pass it down to the
controller.

@pedroabreu sorry, I think I'm misunderstanding you. Are you suggesting a hierarchy like this?:

<DateRangePicker>
  <DayPickerRangeController
    startDate={this.state.startDate} // momentPropTypes.momentObj or null,
    endDate={this.state.endDate} // momentPropTypes.momentObj or null,
    onDatesChange={({ startDate, endDate }) => this.setState({ startDate, endDate })} // PropTypes.func.isRequired,
    focusedInput={this.state.focusedInput} // PropTypes.oneOf([START_DATE, END_DATE]) or null,
    onFocusChange={focusedInput => this.setState({ focusedInput })} // PropTypes.func.isRequired,
    maxdate={moment().endOf('month')}
  />
</DateRangePicker>

ljharb · 2019-03-13T17:09:50Z

I believe they are suggesting making a change in this package itself to pass those props down.

pedroabreu · 2019-03-13T17:15:57Z

Sorry, it's what @ljharb mentioned. These props are not part of the DateRangePicker, only the DateRangePickerController.

I'll open a PR for this, should be pretty straightforward (famous last words)

avivash · 2019-03-13T17:17:41Z

Thanks @ljharb / @pedroabreu. That's great! I'll be sure to look out for a PR

pedroabreu · 2019-03-13T17:33:49Z

PR opened: #1582

avivash · 2019-03-13T17:38:23Z

Thanks for getting this addressed so quickly!

ianduvall · 2019-03-26T16:48:45Z

PR to improve upon the stories created by this PR: #1598

bdrazen · 2020-04-20T20:17:21Z

Does this not work for the SingleDatePicker?

Edit: Nvm, addressed in later PR: #1927

ljharb previously requested changes Aug 15, 2018

View reviewed changes

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

majapw reviewed Aug 15, 2018

View reviewed changes

elinskytap mentioned this pull request Sep 8, 2018

Disable preNav and nextPav #692

Closed

mlg87 approved these changes Sep 13, 2018

View reviewed changes

elinskytap approved these changes Sep 14, 2018

View reviewed changes

Remownz reviewed Sep 19, 2018

View reviewed changes

src/components/DayPickerRangeController.jsx Show resolved Hide resolved

pedroabreu added 11 commits November 17, 2018 01:12

Disable navigation controllers based on min and max date

debfe42

Add stories

aae470f

Early return if not a moment object

1e0a2f0

Tests for block navigation

ea7137a

Use disabled colors in navigation

5380db9

Rename to disablePrev/Next so the default is falsy

9bb571a

disable navigation onKeyUp

a422d40

aria-disabled in navigation button

c209a9a

Rename to shouldDisableMonthNavigation

4693794

Init disablePrev and disableNext in component state

246419c

Fix indentation

9c6ad6c

backwardok reviewed Nov 17, 2018

View reviewed changes

backwardok and others added 3 commits November 17, 2018 14:19

Fallback to undefined in aria-disabled when not disabled

df2fd38

Co-Authored-By: pedroabreu <pmig.abreu@gmail.com>

pre-populate state depending on initial props

49a7a0f

disable onMouseUp event by disable prop

9767c36

ljharb approved these changes Nov 28, 2018

View reviewed changes

backwardok merged commit 37f8aba into react-dates:master Nov 28, 2018

majapw mentioned this pull request Jan 15, 2019

Disable navigation outside of range #1475

Closed

ljharb mentioned this pull request Jan 15, 2019

Option to control total rendered months. #647

Open

minDate and maxDate to block navigation #1311

minDate and maxDate to block navigation #1311

Conversation

pedroabreu commented Aug 14, 2018

coveralls commented Aug 14, 2018 • edited

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

pedroabreu Aug 15, 2018 • edited

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

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

pedroabreu commented Aug 15, 2018 • edited

pedroabreu commented Sep 4, 2018

saadulde45 commented Oct 12, 2018

pedroabreu commented Oct 16, 2018

ljharb commented Oct 19, 2018

pedroabreu commented Nov 17, 2018

backwardok left a comment

Choose a reason for hiding this comment

elinskytap commented Nov 28, 2018

avivash commented Mar 12, 2019 • edited

pedroabreu commented Mar 12, 2019

avivash commented Mar 12, 2019

pedroabreu commented Mar 13, 2019 via email

avivash commented Mar 13, 2019 • edited

ljharb commented Mar 13, 2019

pedroabreu commented Mar 13, 2019

avivash commented Mar 13, 2019

pedroabreu commented Mar 13, 2019

avivash commented Mar 13, 2019

ianduvall commented Mar 26, 2019

bdrazen commented Apr 20, 2020 • edited

coveralls commented Aug 14, 2018 •

edited

pedroabreu Aug 15, 2018 •

edited

pedroabreu commented Aug 15, 2018 •

edited

avivash commented Mar 12, 2019 •

edited

avivash commented Mar 13, 2019 •

edited

bdrazen commented Apr 20, 2020 •

edited