Fix #4076: Trigger onCalendarClose event and onChange even when the same date is selected as the start and the end date in a date range

[balajis-qb]

Closes: #4076

This ticket fixes the following issues

1. Previously we were checking whether to emit the onCalendarClose close or not using the isBefore function but the issue is the startDate has the default time added, but the endDate we get from the calendar component doesn't have any time added to it, hence the isBefore check was failing as the startDate with some time is always ahead of the same date (endDate) without time
   2. Similarly we were using the same isBefore function to decide whether to consider the date as startDate or endDate for onChange event handler, as the isBefore will fail because of the few milli-sec difference and the onChange call will fail. As a result, the end date (the currently selected date) is being considered as the startDate, hence we need to once again select the endDate value

I fixed the above issues by only comparing the date and not considering the time.

[pullrequest]

✅ This pull request was sent to the PullRequest network.

---

@balajis-qb you can click here to see the review status or cancel the code review job.

[pullrequest]

PullRequest Breakdown

Reviewable lines of change

+ 194
- 3

77% JavaScript (tests)
23% JavaScript

Type of change

Fix - These changes are likely to be fixing a bug or issue.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (57c0438) 95.30% compared to head (0188371) 95.41%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4394      +/-   ##
==========================================
+ Coverage   95.30%   95.41%   +0.11%     
==========================================
  Files          28       29       +1     
  Lines        2491     2508      +17     
  Branches     1024     1025       +1     
==========================================
+ Hits         2374     2393      +19     
+ Misses        117      115       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pullrequest]

Just one question on the intent of a string.

Steven S

---

Reviewed with ❤️ by PullRequest

[pullrequest]

I'm not able to wrap my head around why a day would be 3 digits or why this padding makes sense... is this correct?

🔺 Bug (Critical)

Steven S

[balajis-qb]

Hi Steven,
Actually we're using 3-digit numbers for our CSS class names. Attaching the below screenshot for your reference.

[pullrequest]

I don't think this issue is properly solved by this change (read: you'd have to copy/paste this change each and every time you use the calendar). The root cause of the problem is that this.props.startDate and this.props.endDate come in with different conventions, with startDate being the current Date and endDate being today's midnight Date. I can't see the producer of these props, but solving for this discrepancy would avoid this logic leakage with getDateWithoutTime which is only useful to patch this one issue (and should probably be named getMidnightDate).

Jacques

---

Reviewed with ❤️ by PullRequest

[pullrequest]

PullRequest reviewed the updates made to #4394 up until the latest commit (cc49198). No further issues were found.

Reviewed by:

Jacques

[pullrequest]

✖️ This code review was cancelled. See Details

[balajis-qb]

I don't think this issue is properly solved by this change (read: you'd have to copy/paste this change each and every time you use the calendar). The root cause of the problem is that this.props.startDate and this.props.endDate come in with different conventions, with startDate being the current Date and endDate being today's midnight Date. I can't see the producer of these props, but solving for this discrepancy would avoid this logic leakage with getDateWithoutTime which is only useful to patch this one issue (and should probably be named getMidnightDate).

Jacques

Reviewed with ❤️ by PullRequest

Hi,
Thank you for your feedback. I moved the logic I used to a reusable helper called isDateBefore, which will only compare the dates based on it's mid-night time. The usage of this function is similar to isBefore function. I also renamed getDateWithoutTime to getMidnightDate. Kindly verify and let me know if any changes required.

[martijnrusschen]

Pullrequest team, can you give this a final review?

[guanpu]

As the original issue-raiser, I really want A fix being merged, but from above comment:

The root cause of the problem is that this.props.startDate and this.props.endDate come in with different conventions

If that's true, even though this change might fix the issue, it feels like it's not the best way to fix it. Can we review the logic of how the defaults of endDate is set?
Anyway, I lack understanding of the code base, so feel free to ignore my comment if it mislead the discussion, also ping core team to have a review of this fix.
Thank you @balajis-qb for the effort.

[pullrequest]

✅ This pull request was sent to the PullRequest network.

---

@balajis-qb you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

[pullrequest]

Would you not want to throw an exception if one tries to compare an invalid date?

🔹 Error Handling (Nice to have)

Jacques

[balajis-qb]

Currently, we are returning false, which is technically correct.  Throwing an exception would make it even more a better implementation I guess.  I'll update the change.

[balajis-qb]

I updated the cod to throw an error when it receives an invalid date

[pullrequest]

I think rounding to the nearest date is maybe a little too blunt and unnecessary but I might not be understanding how you're using this. It seems like you should be able to rely on date-fns more than you are.

Andy W

---

Reviewed with ❤️ by PullRequest

[pullrequest]

so i'm guessing this returns either -1, 0 or 1 like the datefns functions compareAsc etc? I am not familiar with your implementation of isBefore

it seems from your PR description that the issue is just that sometimes a difference of a few milliseconds causes the events to fire incorrectly. It seems like rounding to the nearest day is a rather blunt instrument for such a small difference.

🔸 Bug (Important)

Andy W

[balajis-qb]

Hi Andy,

Thank you for your comment.  I'll quickly describe the issue, the calender is not automatically getting closed when the startDate and the endDate of the date range are same, the dates are getting selected, but users need to click outside to close the calendar.  This issue will only come when there is a default value for the startDate with time, means we have some time attached to the startDate, but the dates emitted from the calendar component don't have time.  That was the issue.

Suppose if there is no default date set for the startDate or there is a default date with no time, in that case the existing code will work, because we don't need to append any time at the later part of the time, so it'll work.

But the issue comes when we have a default startDate with time (in the specified example in the issue, as it's Date.now() will give current date with time), because to decide whether we need to close the calendar or not, we'll be checking whether the selected endDate occurs before the startDate.  Here as the startDate has the time which is passed to it from the props, the startDate looks greater than the endDate, and the close operation will be skipped.  But for the calendar close operation, why we need to care about the time, because there the language is only in date and not in time.  Hence I added a new helper called isDateBefore.  We already have a helper from the date-fns called isBefore, isBefore will check whether one date is before another date, it also includes time in its comparision.  But in our case, we just need to take the date part of it, to check whether a selected date is before or after the currently selected endDate.  So, I create this new helper called isDateBefore, which will round the time part of the dates to the mindNight time and compare, so here the date will be retained, but just the time will be rounded which will help us for our comparision.

[balajis-qb]

Hello Everyone,

Thank you everyone for your review of my fix. By checking all the reviews, I think I didn't add the proper description to justify my change. Most of the reviewers asked regarding the new helper isDateBefore. I'll once again explain the issue and the fix I made.

What is the reported issue?

@guanpu reported that the onCalendarClose is not called when selecting the same day as start&end date in selectsRange mode. As a result of, it even after we selected both the startDate and the endDate, the users needs to close the calendar manually by clicking outside

When the issue won't occur?

Even if the reporter @guanpu mentioned the onCalendarClose is not getting called when the same day is selected for both the startDate and the endDate, that's not the case always. For example in the following case, the issue won't occur with our existing code itself

() => {
  const [startDate, setStartDate] = React.useState(); // No default date is set
  const [endDate, setEndDate] = React.useState();
  const onChange = (dates) => {
    const [start, end] = dates;
    setStartDate(start);
    setEndDate(end);
  };
  const handleCalendarClose = () => console.log("Calendar closed");

  return (
    <DatePicker
      selected={startDate}
      onChange={onChange}
      startDate={startDate}
      endDate={endDate}
      shouldCloseOnSelect={true}
      onCalendarClose={handleCalendarClose}
      selectsRange
    />
  );
};

If you check the above code, I didn't set any default value for the startDate (& also the endDate). In this case, the issue won't occur. There is one another case

() => {
  const defaultStartDate = new Date("2024-02-01T00:00:00");

  const [startDate, setStartDate] = React.useState(defaultStartDate); // The default date has no time attached
  const [endDate, setEndDate] = React.useState();
  const onChange = (dates) => {
    const [start, end] = dates;
    
    console.log(dates)
    setStartDate(start);
    setEndDate(end);
  };
  const handleCalendarClose = () => console.log("Calendar closed");

  return (
    <DatePicker
      selected={startDate}
      onChange={onChange}
      startDate={startDate}
      endDate={endDate}
      shouldCloseOnSelect={true}
      onCalendarClose={handleCalendarClose}
      selectsRange
    />
  );
};

In the above case, the issue won't occur as the defaultStartDate don't have time.

When the issue occurs?

So, the issue occurs only in the case where we set some startDate and that also has some time attached to it like the example mentioned by @guanpu in the issue or like below

() => {
  const defaultStartDate = new Date();

  const [startDate, setStartDate] = React.useState(defaultStartDate);
  const [endDate, setEndDate] = React.useState();
  const onChange = (dates) => {
    const [start, end] = dates;
    
    console.log(dates)
    setStartDate(start);
    setEndDate(end);
  };
  const handleCalendarClose = () => console.log("Calendar closed");

  return (
    <DatePicker
      selected={startDate}
      onChange={onChange}
      startDate={startDate}
      endDate={endDate}
      shouldCloseOnSelect={true}
      onCalendarClose={handleCalendarClose}
      selectsRange
    />
  );
};

What causes the issue?

Whenever we select any date from the calendar component, obviously the calendar component won't attach any time value to it. So, it'll be in the format like new Date("2024-02-01T00:00:00");, later in our code we'll append the time value (from the default date we passed to it) to this date in the setSelected method

Whenever we select a date, at first the handleSelect event handler will get called. In that handler only we decide whether to close the calendar or not based on the previously selected StartDate and the currently selected endDate (check the highlighted code block from the below screenshot).

Later the setSelected method will get called, which will preserve the time value we set from the default startDate or endDate to the corresponding selected start/end date.

There are 2 cases now

1. User selects the same date as endDate (as the startDate value is already there, the new date will be treated as endDate only)
   In this case, the newly selected endDate which we got from the calendar component don't have any time value attached. In the handleSelect which will be called before setSelected we check the new date without time not occurs before the startDate. For example 2024-02-01T2:10:43 < 2024-02-01T00:00:00 (startDate < selected endDate)

2. User selects a new startDate (by selecting the endDate first, then selecting the other startDate than the default date that was set)
   In this case also the newly selected startDate will retain the time value from the default startDate by the setSelected function.
   

   As a result of it, when we receive the same date as the endDate, on handleSelect, the check will be failed, because the isBefore will compare the date by both it's date and time

What is my solution?

I solved this issue in the following way. The language of the calendar will be in the day format and not in the time format. The date we received from the calendar component on handleSelect has date with 00:00:00 time (12am). So instead of deciding whether to close a calendar or not by both date and time, I decided to do that only by the corresponding dates. Hence I create a new utility method called isDateBefore, which will first set 0 for the time value and then pass it to the isBefore utility that we have already.

isDateBefore Screenshot

Here we solved the issue by setting the same time value for both the dates that we wish to compare (which is 00), but, however, we instead of 00, we can set any constant time value to the dates to make the fix works, because if the time value is same, only difference would is of date.

Hope my explanation answered all your questions. If you have a question like, then do we need to replace all isBefore with isDateBefore, my answer would be No, here we used it as we need to decide whether to keep the calendar open or not in the language of calendar (which is just with date, and not time), hence I used it only here. If any one has any questions or suggestions please let me know.

cc: @martijnrusschen @guanpu

[pullrequest]

PullRequest reviewed the updates made to #4394 up until the latest commit (0188371). No further issues were found.

Reviewed by:

Andy W