-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
Bugfix closing the datepicker should cancel the ongoing selection 1836 #1856
Bugfix closing the datepicker should cancel the ongoing selection 1836 #1856
Conversation
…the-ongoing-selection-1836
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 verified this fix works. If any other changes are needed, please include my suggested changes with that update.
// Reassign the From and To Day | ||
|
||
//If both startDate and endDate were already selected. Start a new range selection. | ||
if(startDate && endDate){ |
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.
nit: add space after if
(lines 175 and 180)
setFromDay(day); | ||
updateEndDate(null); | ||
setEnteredTo(null); | ||
//If startDate is selected and endDate is unselected, complete the range selection. |
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.
nit: for comments, add space between //
and start of comment
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.
Reviewed the changes and it does resolve the issue brought by #1836
LGTM
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.
@DorianDeptuch Great job on this issue! Your code passed the ABC's, including:
- PR is made from the correct branch
- PR links to the correct issue:
- The correct files were edited
- No extra edits were made
- Appearance of the website looks good (Chrome and Firefox)
- I agree with the nitpicks from Kelly, if anything else needs changing.
Approved! 👍
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.
@DorianDeptuch feel free to make the "nit" adjustments, otherwise this is looking good 👍
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.
Approved. We're ready to punch this through 👍
Merge when ready
Fixes #1836
main
branchAny questions? See the getting started guide