-
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
Merged
martijnrusschen
merged 6 commits into
Hacker0x01:main
from
qburst:issue-4076/fix/emit-onCalendarClose-onsame-range-select
Jan 4, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e3d65b2
fix: Trigger onCalendarClose and onClose event when the same date is …
cc49198
🚚: Rename getDateWithoutTime to getMidnightDate for more readability
cd9fb9d
✨ Add getMidnightDate and isDateBefore helpers and test cases
d2e7531
🔨 Reuse the shared helper isBeforeDate to compare the dates based on …
c1ae2fa
feature: Update the isDateBefore to throw an exception when either of…
0188371
Merge branch 'main' into issue-4076/fix/emit-onCalendarClose-onsame-r…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
|
@@ -2019,6 +2031,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", () => { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
so i'm guessing this returns either -1, 0 or 1 like the
datefns
functionscompareAsc
etc? I am not familiar with your implementation ofisBefore
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
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.
Hi Andy,
Thank you for your comment. I'll quickly describe the issue, the calender is not automatically getting closed when the
startDate
and theendDate
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.