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

Calendar Modal Fix - Multiple Selections Display #471

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Atikpui007
Copy link
Collaborator

Summary

This pull request fixes the bug displayed on the Mini Calendar when selecting dates using the Today and Tomorrow buttons. Also addresses closing the modal when a date is selected (ideally not the intended behavior)

  • implemented useRef state to address the change to the DatePicker component
  • fixed closing modal when date is selected

Test Plan

Choose dates using the Today and Tomorrow buttons when other dates are selected to identify behavior

Notes

Initial behavior

Screen.Recording.2023-10-22.at.13.25.08.mov

Fixed behavior

Screen.Recording.2023-10-22.at.13.23.53.mov

Breaking Changes

None

@Atikpui007 Atikpui007 requested a review from a team as a code owner October 22, 2023 17:36
@CLAassistant
Copy link

CLAassistant commented Oct 22, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Atikpui007
❌ Atikpui7
You have signed the CLA already but the status is still pending? Let us recheck it.

@dti-github-bot
Copy link
Member

dti-github-bot commented Oct 22, 2023

[diff-counting] Significant lines: 47.

@Atikpui007 Atikpui007 self-assigned this Oct 22, 2023
Enochen
Enochen previously approved these changes Oct 22, 2023
Copy link
Contributor

@Enochen Enochen left a comment

Choose a reason for hiding this comment

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

Works on my machine but have few suggestions

@@ -33,6 +33,7 @@ const Icon = () => (

const MiniCal = () => {
const { curDate, setCurDate } = useDate();
const datePickerRef = React.useRef<any>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can extend the types via

declare module 'react-datepicker' {
  export default class ReactDatePicker {
    setPreSelection?: (date: Date) => void;
  }
}

to be able to do

Suggested change
const datePickerRef = React.useRef<any>(null);
const datePickerRef = React.useRef<DatePicker>(null);

Copy link
Contributor

Choose a reason for hiding this comment

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

imo we don't want to introduce more anys into the system if we don't have to

Comment on lines 110 to 112
if (datePickerRef.current) {
datePickerRef.current.setSelected(today);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional chaining works with function calls too; also setPreSelection seems more appropriate than setSelected here (likely less overhead). It's also what's used in react-datepicker's native implementation of the "Today" button.

Suggested change
if (datePickerRef.current) {
datePickerRef.current.setSelected(today);
}
datePickerRef.current?.setPreSelection?.(today);

Copy link
Contributor

@SGupta101 SGupta101 left a comment

Choose a reason for hiding this comment

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

Great work on this PR Desmond! I think the changes look good, I tested them on different browsers and it seems to work properly. Because the CustomInput function does not have an internal state, it can be converted to a functional component to make the code a bit shorter, but it is not needed.

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.

7 participants