-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
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 #4394
Changes from 4 commits
e3d65b2
cc49198
cd9fb9d
d2e7531
c1ae2fa
0188371
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -862,3 +862,40 @@ | |
export function isSameMinute(d1, d2) { | ||
return startOfMinute(d1).getTime() === startOfMinute(d2).getTime(); | ||
} | ||
|
||
/** | ||
* Returns a cloned date with midnight time (00:00:00) | ||
* | ||
* @param {Date} date The date for which midnight time is required | ||
* @param {Date} dateToCompare the date to compare with | ||
* @returns {Date} A new datetime object representing the input date with midnight time | ||
*/ | ||
export function getMidnightDate(date) { | ||
if (!isDate(date)) { | ||
throw new Error("Invalid date"); | ||
} | ||
|
||
const dateWithoutTime = new Date(date); | ||
dateWithoutTime.setHours(0, 0, 0, 0); | ||
return dateWithoutTime; | ||
} | ||
|
||
/** | ||
* Is the first date before the second one? | ||
* | ||
* @param {Date} date The date that should be before the other one to return true | ||
* @param {Date} dateToCompare The date to compare with | ||
* @returns {boolean} The first date is before the second date | ||
* | ||
* Note: | ||
* This function considers the mid-night of the given dates for comparison. | ||
* It evaluates whether date is before dateToCompare based on their mid-night timestamps. | ||
*/ | ||
export function isDateBefore(date, dateToCompare) { | ||
const midnightDate = isDate(date) ? getMidnightDate(date) : null; | ||
const midnightDateToCompare = isDate(dateToCompare) | ||
? getMidnightDate(dateToCompare) | ||
: null; | ||
|
||
return isBefore(midnightDate, midnightDateToCompare); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so i'm guessing this returns either -1, 0 or 1 like the 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)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Andy, Thank you for your comment. I'll quickly describe the issue, the calender is not automatically getting closed when the 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. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import { findDOMNode } from "react-dom"; | |
import TestUtils from "react-dom/test-utils"; | ||
import { enUS, enGB } from "date-fns/locale"; | ||
import { mount } from "enzyme"; | ||
import { render, fireEvent } from "@testing-library/react"; | ||
import { render, act, waitFor, fireEvent } from "@testing-library/react"; | ||
import defer from "lodash/defer"; | ||
import DatePicker, { registerLocale } from "../src/index.jsx"; | ||
import Day from "../src/day.jsx"; | ||
|
@@ -44,6 +44,18 @@ function goToLastMonth(datePicker) { | |
TestUtils.Simulate.click(findDOMNode(lastMonthButton)); | ||
} | ||
|
||
function formatDayWithZeros(day) { | ||
const dayString = day.toString(); | ||
|
||
if (dayString.length === 1) { | ||
return `00${dayString}`; | ||
} | ||
if (dayString.length === 2) { | ||
return `0${dayString}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
return dayString; | ||
} | ||
|
||
describe("DatePicker", () => { | ||
afterEach(() => { | ||
jest.resetAllMocks(); | ||
|
@@ -1941,6 +1953,94 @@ describe("DatePicker", () => { | |
expect(onChangeSpy.mock.calls[0][0][0]).toBeNull(); | ||
expect(onChangeSpy.mock.calls[0][0][1]).toBeNull(); | ||
}); | ||
|
||
it("should call the onChange even when the startDate and the endDate is same in the range (case when we programmatically set the startDate, but set the same endDate through UI)", async () => { | ||
let startDate = new Date(); | ||
let endDate = null; | ||
|
||
const onChangeSpy = jest.fn(); | ||
|
||
const { container } = render( | ||
<DatePicker | ||
startDate={startDate} | ||
endDate={endDate} | ||
onChange={onChangeSpy} | ||
shouldCloseOnSelect | ||
selectsRange | ||
/>, | ||
); | ||
|
||
const input = container.querySelector("input"); | ||
expect(input).toBeTruthy(); | ||
fireEvent.click(input); | ||
|
||
let calendar = container.querySelector(".react-datepicker"); | ||
expect(calendar).toBeTruthy(); | ||
|
||
// Select the same date as the start date | ||
const startDatePrefixedWithZeros = formatDayWithZeros( | ||
startDate.getDate(), | ||
); | ||
const endDateElement = container.querySelector( | ||
`.react-datepicker__day--${startDatePrefixedWithZeros}`, | ||
); | ||
fireEvent.click(endDateElement); | ||
|
||
await act(async () => { | ||
await waitFor(() => { | ||
expect(onChangeSpy).toHaveBeenCalled(); | ||
}); | ||
}); | ||
}); | ||
|
||
it("should hide the calendar even when the startDate and the endDate is same in the range", async () => { | ||
let startDate = new Date(); | ||
let endDate = null; | ||
|
||
const onCalendarCloseSpy = jest.fn(); | ||
|
||
const onChange = (dates) => { | ||
const [start, end] = dates; | ||
startDate = start; | ||
endDate = end; | ||
}; | ||
|
||
const { container } = render( | ||
<DatePicker | ||
startDate={startDate} | ||
endDate={endDate} | ||
onChange={onChange} | ||
onCalendarClose={onCalendarCloseSpy} | ||
shouldCloseOnSelect | ||
selectsRange | ||
/>, | ||
); | ||
|
||
const input = container.querySelector("input"); | ||
expect(input).toBeTruthy(); | ||
fireEvent.click(input); | ||
|
||
let calendar = container.querySelector(".react-datepicker"); | ||
expect(calendar).toBeTruthy(); | ||
|
||
// Select the same date as the start date | ||
const startDatePrefixedWithZeros = formatDayWithZeros( | ||
startDate.getDate(), | ||
); | ||
const endDateElement = container.querySelector( | ||
`.react-datepicker__day--${startDatePrefixedWithZeros}`, | ||
); | ||
fireEvent.click(endDateElement); | ||
|
||
await act(async () => { | ||
await waitFor(() => { | ||
calendar = container.querySelector(".react-datepicker"); | ||
expect(calendar).toBeFalsy(); | ||
|
||
expect(onCalendarCloseSpy).toHaveBeenCalled(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
describe("duplicate dates when multiple months", () => { | ||
|
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.
Would you not want to throw an exception if one tries to compare an invalid date?
🔹 Error Handling (Nice to have)
Jacques
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.
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.
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 updated the cod to throw an error when it receives an invalid date