Reset after-hovered-start in componentWillReceiveProps - Fixes #831#843
Reset after-hovered-start in componentWillReceiveProps - Fixes #831#843
Conversation
wyattdanger
added
the
semver-patch: fixes/refactors/etc
There was a problem hiding this comment.
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
| 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.
I wonder if we should do something like
const {
startDate: prevStartDate,
minimumNights: prevMinimumNights,
...
} = this.propsso that we're not referencing this.props so many times in this function
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
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.
wyattdanger
force-pushed
the
swb--dayrangepicker-ghost-values
branch
from
cee2e7b to
1160090
Compare
|
@backwardok @erin-doyle alright, all this.props references in componentWillReceiveProps have been destructured in a second commit |
6 participants
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