-
-
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
Reset after-hovered-start in componentWillReceiveProps - Fixes #831 #843
Conversation
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.
Don't know that I know this library well enough to definitively approve it (maybe @ljharb does?) but the gif seems like better behavior. What it is highlighting to me is that the after-hovered-start
doesn't make as much sense in its current state in a keyboard world, and should probably be added as you move around the calendar in the same way that it is if you were hovering. However, that's perhaps out of scope for this PR (but seems worth a separate PR to really tackle the desired UX)
Also want to verify that you also tested that this is not broken for mouse users
@@ -258,6 +258,11 @@ export default class DayPickerRangeController extends React.Component { | |||
if (didStartDateChange) { | |||
modifiers = this.deleteModifier(modifiers, this.props.startDate, 'selected-start'); | |||
modifiers = this.addModifier(modifiers, startDate, 'selected-start'); | |||
if (this.props.startDate) { | |||
const startSpan = this.props.startDate.clone().add(1, 'day'); |
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 wonder if we should do something like
const {
startDate: prevStartDate,
minimumNights: prevMinimumNights,
...
} = this.props
so that we're not referencing this.props
so many times in this function
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 seems reasonable, but let's get some more eyes on it if possible before merging.
Oh, hello again @erin-doyle! |
Isn't it a small React world @wyattdanger ! I think this looks good besides the suggestion to unpack this.prop. |
after-hovered-start was being unset in onDayClick, which led to an issue where the modifier was left as applied when changing dates via keyboard navigation. This moves that logic into componentWillReceiveProps so that it works in both cases.
cee2e7b
to
1160090
Compare
@backwardok @erin-doyle alright, all this.props references in componentWillReceiveProps have been destructured in a second commit |
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.
LGTM! Thanks! 👍
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.
LGTM! Thanks!
after-hovered-start was being unset in onDayClick, which led to an issue
where the modifier was left as applied when changing dates via keyboard
navigation. This moves that logic into componentWillReceiveProps so that
it works in both cases.
Fixes #831
Gif of keyboard navigation and changing dates after the fix, for comparison with gif in the issue:
cc @backwardok @majapw