-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Recalculate blocked days when day-blocking function prop changes #668
Conversation
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). |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💣 .com
if you rebase/squash it could clean up the commits, if you like. |
cece6c0
to
ab1f2f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\o/
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
, orisDayHighlighted
props change.Alex Meed