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

Recalculate blocked days when day-blocking function prop changes #668

Merged

Conversation

MathSquared
Copy link

Sometimes it's useful to force the picker to recompute which days should be blocked or highlighted (for instance, if we need to force the set of blocked days to change based on data fetched asynchronously). This change forces the picker to refetch the blocked/highlighted modifiers for all visible dates when the isOutsideRange, isDayBlocked, or isDayHighlighted props change.

Alex Meed

@timhwang21
Copy link
Collaborator

timhwang21 commented Aug 8, 2017

This is sweet, would love to see this. Falls in the same vein as #652, with I think the same performance concern (though to a lesser degree since I'd expect these props to change less than focus).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 86.324% when pulling 1c854cd on MathSquared:alex-meed-recalc-blocked-dates into d32e9b3 on airbnb:master.

@majapw majapw self-requested a review August 8, 2017 23:06
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.

This may seem like a hyper optimization, but we may want to have a variable for each prop modifier, iterate through the visible days if any of the vars are true or the focus has changed and then do the recalculation only as needed for each one. What do you think?

@@ -187,16 +187,21 @@ export default class DayPickerRangeController extends React.Component {
} = nextProps;
let { visibleDays } = this.state;

let recomputeBlockedDays = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather have this be named something like recomputePropModifiers since it is recomputing more than just the blocked days.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe we even want to separate out this logic such that we're only recalculating the functions that we need to? (e.g. only recompute outside days, blocked days, etc.)

@MathSquared
Copy link
Author

Sounds reasonable—made such a change in e17b8a1.

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

Choose a reason for hiding this comment

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

One final nit - to maintain the behavior from before, we want this to be if (recomputeOutsideRange || didFocusChange) {. Same for all the ones below.

Copy link
Author

Choose a reason for hiding this comment

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

✅  Fixed (hoping the superfluous merge commit cece6c0 doesn't screw anything up).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.443% when pulling e17b8a1 on MathSquared:alex-meed-recalc-blocked-dates into d32e9b3 on airbnb:master.

majapw
majapw previously approved these changes Aug 9, 2017
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.

💣 .com

@majapw
Copy link
Collaborator

majapw commented Aug 9, 2017

if you rebase/squash it could clean up the commits, if you like.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.473% when pulling 41793ab on MathSquared:alex-meed-recalc-blocked-dates into 00164cb on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.473% when pulling cece6c0 on MathSquared:alex-meed-recalc-blocked-dates into 00164cb on airbnb:master.

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.

\o/

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.473% when pulling ab1f2f5 on MathSquared:alex-meed-recalc-blocked-dates into 00164cb on airbnb:master.

@majapw majapw merged commit 06345e5 into react-dates:master Aug 9, 2017
@MathSquared MathSquared deleted the alex-meed-recalc-blocked-dates branch August 9, 2017 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants