Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SingleDatePicker now re-evaluates isOutsideRange on componentWillReceiveProps #550

Merged
merged 4 commits into from
Jun 12, 2017

Conversation

jlyman
Copy link
Contributor

@jlyman jlyman commented Jun 8, 2017

Fixes #549 by adding a new call to isOutsideRange on componentWillReceiveProps, just like what is being done for isDayBlocked and isDayHighlighted.

@@ -218,6 +218,12 @@ export default class SingleDatePicker extends React.Component {
values(visibleDays).forEach((days) => {
Object.keys(days).forEach((day) => {
const momentObj = moment(day);
if (isOutsideRange(momentObj)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need to account for this.isBlocked() as well, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate for me @timhwang21? There are a couple other checks (isDayBlocked() and isDayHighlighted() that happen right afterwards, and I believe this.isBlocked() checks both isDayBlocked and isOutsideRange (on line 628).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have to admit my memory is a bit fuzzy on this now but I believe isBlocked adds its own modifier on top of the others. So you can successfully remove the modifiers for isDayBlocked and isOutsideRange but still have the calendar day blocked. This is what i remember from my own pr, hopefully one of the maintainers can clarify.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What @timhwang21 is saying is we should update this component to look more like what's in the DayPickerRangeController, ie:

if (didFocusChange) {
      values(visibleDays).forEach((days) => {
        Object.keys(days).forEach((day) => {
          const momentObj = moment(day);

          if (this.isBlocked(momentObj)) {
            modifiers = this.addModifier(modifiers, momentObj, 'blocked');
          } else {
            modifiers = this.deleteModifier(modifiers, momentObj, 'blocked');
          }

          if (isOutsideRange(momentObj)) {
            modifiers = this.addModifier(modifiers, momentObj, 'blocked-out-of-range');
          } else {
            modifiers = this.deleteModifier(modifiers, momentObj, 'blocked-out-of-range');
          }

          if (isDayBlocked(momentObj)) {
            modifiers = this.addModifier(modifiers, momentObj, 'blocked-calendar');
          } else {
            modifiers = this.deleteModifier(modifiers, momentObj, 'blocked-calendar');
          }

          if (isDayHighlighted(momentObj)) {
            modifiers = this.addModifier(modifiers, momentObj, 'highlighted-calendar');
          } else {
            modifiers = this.deleteModifier(modifiers, momentObj, 'highlighted-calendar');
          }
        });
      });
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotchya... Thanks @majapw and @timhwang21, that makes sense. Let me go ahead and make that improvement.

@majapw
Copy link
Collaborator

majapw commented Jun 8, 2017

This looks good! Just the one comment on adding the this.isBlocked => --blocked modifier as well.

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@majapw majapw merged commit 8f2fed0 into react-dates:master Jun 12, 2017
@vebits
Copy link

vebits commented Jun 16, 2017

Is this feature in v12.1.2?

@jlyman
Copy link
Contributor Author

jlyman commented Jun 16, 2017

@vebjorni I don't believe so, as 12.1.2 was cut seven days ago, and this was merged 4 days ago.

@vebits
Copy link

vebits commented Jun 19, 2017

@jlyman Thanks, that makes sense.

@majapw Any idea when the next version will be released?

@vebits vebits mentioned this pull request Jun 23, 2017
@majapw majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Jun 28, 2017
timhwang21 added a commit to timhwang21/react-dates that referenced this pull request Jun 30, 2017
(Rebased and re-added changes in react-dates#550)

Addresses react-dates#570.

Currently, block / range related modifiers are only recomputed when the currently focused input changes. However, this causes some issues when more complex block / range functions are used, e.g. those that make selectable dates conditional on current state.

This commit gives one potential fix, which is to recompute modifiers not just on focus change, but also on start / end date change. This DOES result in a slight perf cost because modifiers are computed more frequently; however from my experience perf bottlenecks have mainly arisen on month changes (where 30 new days are added and recomputed) and on hover changes (fixed in v11.x), not on selection change.

The added story gives an example of a case where range modifier computation does not work as expected. The added wrapper component has the following validation: start dates must be no earlier than 1 week after selected end date, and end dates must be no later than 1 week after selected start date. Without the change in `DayPickerRangeController`, the following actions lead to a bug:

1. Pick a start date. Note the modifiers correctly recompute after the end date is focused.
2. Click on a date BEFORE the start date. `react-dates` behavior is to change the currently selected start date. However, note that the modifiers DO NOT recompute.

Additionally, there is a similar bug when `keepOpenOnDateSelect` is enabled, which is more noticeable with the `SingleDatePicker`. Because modifiers recompute when the focused input toggles between `null` and the input, modifiers are never recomputed until the calendar is closed and reopened. The story in `SingleDatePicker` showcases this -- when the change in `SingleDatePicker.jsx` is undone, the modifier never recomputes.

If this change seems ok I'll delete the changes to unrelated files, and add test cases. Thanks!
majapw pushed a commit that referenced this pull request Jun 30, 2017
majapw pushed a commit that referenced this pull request Jun 30, 2017
timhwang21 added a commit to timhwang21/react-dates that referenced this pull request Jul 29, 2017
(Rebased and re-added changes in react-dates#550)

Addresses react-dates#570.

Currently, block / range related modifiers are only recomputed when the currently focused input changes. However, this causes some issues when more complex block / range functions are used, e.g. those that make selectable dates conditional on current state.

This commit gives one potential fix, which is to recompute modifiers not just on focus change, but also on start / end date change. This DOES result in a slight perf cost because modifiers are computed more frequently; however from my experience perf bottlenecks have mainly arisen on month changes (where 30 new days are added and recomputed) and on hover changes (fixed in v11.x), not on selection change.

The added story gives an example of a case where range modifier computation does not work as expected. The added wrapper component has the following validation: start dates must be no earlier than 1 week after selected end date, and end dates must be no later than 1 week after selected start date. Without the change in `DayPickerRangeController`, the following actions lead to a bug:

1. Pick a start date. Note the modifiers correctly recompute after the end date is focused.
2. Click on a date BEFORE the start date. `react-dates` behavior is to change the currently selected start date. However, note that the modifiers DO NOT recompute.

Additionally, there is a similar bug when `keepOpenOnDateSelect` is enabled, which is more noticeable with the `SingleDatePicker`. Because modifiers recompute when the focused input toggles between `null` and the input, modifiers are never recomputed until the calendar is closed and reopened. The story in `SingleDatePicker` showcases this -- when the change in `SingleDatePicker.jsx` is undone, the modifier never recomputes.

If this change seems ok I'll delete the changes to unrelated files, and add test cases. Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants