-
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
Select multiple non-consecutive dates #4537
Changes from 17 commits
b2cca19
a776678
5052d88
52603c2
0df17aa
40b5f05
a5225ec
7092c09
9593bcb
f6e2d80
3151397
4d3fbf0
7173fb3
f65ab37
22d608c
e0a7948
0abfacc
0a1d636
a148c36
ee931c5
effa529
4cf8db1
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 |
---|---|---|
@@ -0,0 +1,15 @@ | ||
() => { | ||
const [selectedDates, setSelectedDates] = useState([new Date()]); | ||
const onChange = (dates) => { | ||
setSelectedDates(dates); | ||
}; | ||
return ( | ||
<DatePicker | ||
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. ISSUE: react/jsx-no-undef (Severity: Medium) Remediation: 🤖 powered by PullRequest Automation 👋 verified by Ryan Lester <@buu700>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. It's the same for all the example files, none of them import DatePicker 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. Ryan Lester <@buu700> |
||
selectedDates={selectedDates} | ||
selectsMultiple | ||
onChange={onChange} | ||
shouldCloseOnSelect={false} | ||
disabledKeyboardNavigation | ||
/> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,6 +197,23 @@ export function safeDateRangeFormat(startDate, endDate, props) { | |
return `${formattedStartDate} - ${formattedEndDate}`; | ||
} | ||
|
||
export function safeMultipleDatesFormat(dates, props) { | ||
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. Seems like a lot of care was put into defensive input handling here. ◽ ComplimentRyan Lester <@buu700> 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. If length is zero, it should return an empty string. I haven't looked that deep into the underlying code to see if null / undefined is a valid input. But I'm used to do the ?.length test in our own code, so seemed natural for me to do that. Now that I think of it, it could probably skip a couple of if branches here, and reduce cyclic complexity a bit, by using an array join, like this export function safeMultipleDatesFormat(dates, props) {
if (dates?.length > 2) {
return `${safeDateFormat(dates[0], props)} (+${dates.length - 1})`;
}
return (dates || []).map((date) => safeDateFormat(date, props)).join(', ')
} 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. Also seems that the rest of the safeDateFormat methods is taking care of possible null/undefined inputs |
||
if (!dates?.length) { | ||
return ""; | ||
} | ||
const formattedFirstDate = safeDateFormat(dates[0], props); | ||
if (dates.length === 1) { | ||
return formattedFirstDate; | ||
} | ||
if (dates.length === 2) { | ||
const formattedSecondDate = safeDateFormat(dates[1], props); | ||
return `${formattedFirstDate}, ${formattedSecondDate}`; | ||
} | ||
|
||
const extraDatesCount = dates.length - 1; | ||
return `${formattedFirstDate} (+${extraDatesCount})`; | ||
} | ||
|
||
// ** Date Setters ** | ||
|
||
export function setTime(date, { hour = 0, minute = 0, second = 0 }) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,8 @@ | |
showWeekPicker: PropTypes.bool, | ||
showWeekNumber: PropTypes.bool, | ||
selectsDisabledDaysInRange: PropTypes.bool, | ||
selectsMultiple: PropTypes.bool, | ||
selectedDates: PropTypes.arrayOf(PropTypes.instanceOf(Date)), | ||
startDate: PropTypes.instanceOf(Date), | ||
renderDayContents: PropTypes.func, | ||
handleOnKeyDown: PropTypes.func, | ||
|
@@ -93,14 +95,32 @@ | |
|
||
isSameDay = (other) => isSameDay(this.props.day, other); | ||
|
||
isKeyboardSelected = () => | ||
!this.props.disabledKeyboardNavigation && | ||
!( | ||
this.isSameDay(this.props.selected) || | ||
this.isSameWeek(this.props.selected) | ||
) && | ||
(this.isSameDay(this.props.preSelection) || | ||
this.isSameWeek(this.props.preSelection)); | ||
isKeyboardSelected = () => { | ||
if (this.props.disabledKeyboardNavigation) { | ||
return false; | ||
} | ||
|
||
if (this.props.selectsMultiple) { | ||
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. This might be slightly more readable as something like:
You could also go further and factor out 🔹 Code Reuse (Nice to have)Ryan Lester <@buu700> |
||
const isSelectedDate = this.props.selectedDates?.some( | ||
(date) => this.isSameDay(date) || this.isSameWeek(date), | ||
); | ||
|
||
return ( | ||
!isSelectedDate && | ||
(this.isSameDay(this.props.preSelection) || | ||
this.isSameWeek(this.props.preSelection)) | ||
); | ||
} | ||
|
||
return ( | ||
!( | ||
this.isSameDay(this.props.selected) || | ||
this.isSameWeek(this.props.selected) | ||
) && | ||
(this.isSameDay(this.props.preSelection) || | ||
this.isSameWeek(this.props.preSelection)) | ||
); | ||
}; | ||
|
||
isDisabled = () => isDayDisabled(this.props.day, this.props); | ||
|
||
|
@@ -276,8 +296,17 @@ | |
|
||
isCurrentDay = () => this.isSameDay(newDate()); | ||
|
||
isSelected = () => | ||
this.isSameDay(this.props.selected) || this.isSameWeek(this.props.selected); | ||
isSelected = () => { | ||
if (this.props.selectsMultiple) { | ||
return this.props.selectedDates?.some( | ||
(date) => this.isSameDay(date) || this.isSameWeek(this.props.selected), | ||
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. This looks like a bug: In addition to fixing the bug, consider adding a test to cover this specific issue. 🔺 Bug (Critical)Ryan Lester <@buu700> 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. I added this.isSameWeek() as it apparently was used in "single selected" mode, and thought it was supposed to be there? (I'm fairly new to this react component, so bear with me if I'm mistaken ;)) 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. oooh.. now I see it.. yup, that should be date and not props.selected :) 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. Sorry, just to clarify, it seems to me from context that the correct argument to Ryan Lester <@buu700> |
||
); | ||
} | ||
return ( | ||
this.isSameDay(this.props.selected) || | ||
this.isSameWeek(this.props.selected) | ||
); | ||
}; | ||
|
||
getClassNames = (date) => { | ||
const dayClassName = this.props.dayClassName | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ | |
isSameDay, | ||
isMonthDisabled, | ||
isYearDisabled, | ||
safeMultipleDatesFormat, | ||
getHolidaysMap, | ||
isDateBefore, | ||
} from "./date_utils"; | ||
|
@@ -232,6 +233,8 @@ | |
selectsStart: PropTypes.bool, | ||
selectsRange: PropTypes.bool, | ||
selectsDisabledDaysInRange: PropTypes.bool, | ||
selectsMultiple: PropTypes.bool, | ||
selectedDates: PropTypes.arrayOf(PropTypes.instanceOf(Date)), | ||
showMonthDropdown: PropTypes.bool, | ||
showPreviousMonths: PropTypes.bool, | ||
showMonthYearDropdown: PropTypes.bool, | ||
|
@@ -623,12 +626,20 @@ | |
} | ||
} | ||
|
||
const { onChange, selectsRange, startDate, endDate } = this.props; | ||
const { | ||
onChange, | ||
selectsRange, | ||
startDate, | ||
endDate, | ||
selectsMultiple, | ||
selectedDates, | ||
} = this.props; | ||
|
||
if ( | ||
!isEqual(this.props.selected, changedDate) || | ||
this.props.allowSameDay || | ||
selectsRange | ||
selectsRange || | ||
selectsMultiple | ||
) { | ||
if (changedDate !== null) { | ||
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. This would be a pre-existing issue, but is there any scenario where 🔹 Bug (Nice to have)Ryan Lester <@buu700> 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. Not sure about this one? haven't touched this part myself.. 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. Ah okay, sounds good, just something that caught my attention while I was working backwards from your code to see what Ryan Lester <@buu700> |
||
if ( | ||
|
@@ -669,6 +680,24 @@ | |
if (isRangeFilled) { | ||
onChange([changedDate, null], event); | ||
} | ||
} else if (selectsMultiple) { | ||
if (!selectedDates?.length) { | ||
onChange([changedDate], event); | ||
} else { | ||
const isChangedDateAlreadySelected = selectedDates.some( | ||
(selectedDate) => isSameDay(selectedDate, changedDate), | ||
); | ||
|
||
if (isChangedDateAlreadySelected) { | ||
const nextDates = selectedDates.filter( | ||
(selectedDate) => !isSameDay(selectedDate, changedDate), | ||
); | ||
|
||
onChange(nextDates, event); | ||
} else { | ||
onChange([...selectedDates, changedDate], event); | ||
} | ||
} | ||
} else { | ||
onChange(changedDate, event); | ||
} | ||
|
@@ -1018,6 +1047,8 @@ | |
selectsStart={this.props.selectsStart} | ||
selectsEnd={this.props.selectsEnd} | ||
selectsRange={this.props.selectsRange} | ||
selectsMultiple={this.props.selectsMultiple} | ||
selectedDates={this.props.selectedDates} | ||
startDate={this.props.startDate} | ||
endDate={this.props.endDate} | ||
excludeDates={this.props.excludeDates} | ||
|
@@ -1201,7 +1232,9 @@ | |
this.props.endDate, | ||
this.props, | ||
) | ||
: safeDateFormat(this.props.selected, this.props); | ||
: this.props.selectsMultiple | ||
? safeMultipleDatesFormat(this.props.selectedDates, this.props) | ||
: safeDateFormat(this.props.selected, this.props); | ||
|
||
return React.cloneElement(customInput, { | ||
[customInputRef]: (input) => { | ||
|
@@ -1242,10 +1275,14 @@ | |
clearButtonTitle, | ||
clearButtonClassName = "", | ||
ariaLabelClose = "Close", | ||
selectedDates, | ||
} = this.props; | ||
if ( | ||
isClearable && | ||
(selected != null || startDate != null || endDate != null) | ||
(selected != null || | ||
startDate != null || | ||
endDate != null || | ||
selectedDates?.length) | ||
) { | ||
return ( | ||
<button | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import React from "react"; | ||
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. |
||
import DatePicker from "../src"; | ||
import { render } from "@testing-library/react"; | ||
|
||
describe("Multiple Dates Selected", function () { | ||
function getDatePicker(extraProps) { | ||
return render( | ||
<DatePicker | ||
selectsMultiple | ||
onChange={() => {}} | ||
shouldCloseOnSelect={false} | ||
disabledKeyboardNavigation | ||
{...extraProps} | ||
/>, | ||
); | ||
} | ||
|
||
it("should handle text format for no selected date", () => { | ||
const datePicker = getDatePicker({ | ||
selectsMultiple: true, | ||
selectedDates: [], | ||
}); | ||
|
||
expect(datePicker.getByRole("textbox").value).toBe(""); | ||
}); | ||
|
||
it("should handle text format for one selected date", () => { | ||
const datePicker = getDatePicker({ | ||
selectsMultiple: true, | ||
selectedDates: [new Date("2024/01/01")], | ||
}); | ||
|
||
expect(datePicker.getByRole("textbox").value).toBe("01/01/2024"); | ||
}); | ||
|
||
it("should handle text format for two selected dates", () => { | ||
const datePicker = getDatePicker({ | ||
selectsMultiple: true, | ||
selectedDates: [new Date("2024/01/01"), new Date("2024/01/15")], | ||
}); | ||
|
||
expect(datePicker.getByRole("textbox").value).toBe( | ||
"01/01/2024, 01/15/2024", | ||
); | ||
}); | ||
|
||
it("should handle text format for more than two selected dates", () => { | ||
const datePicker = getDatePicker({ | ||
selectsMultiple: true, | ||
selectedDates: [ | ||
new Date("2024/01/01"), | ||
new Date("2024/01/15"), | ||
new Date("2024/03/15"), | ||
], | ||
}); | ||
|
||
expect(datePicker.getByRole("textbox").value).toBe("01/01/2024 (+2)"); | ||
}); | ||
}); |
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.
ISSUE: no-unused-expressions (Severity: Medium)
Expected an assignment or function call and instead saw an expression.
Remediation:
This may be a non-issue if there's an existing convention and bundler configuration that I'm unaware of, but should this not be exported?
🤖 powered by PullRequest Automation 👋 verified by Ryan Lester <@buu700>