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

Support selecting multiple dates #3999

Merged
merged 11 commits into from
Feb 28, 2024
5 changes: 5 additions & 0 deletions docs-site/src/components/Examples/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ import selectsRangeWithDisabledDates from "../../examples/selectsRangeWithDisabl
import CalendarStartDay from "../../examples/calendarStartDay";
import ExternalForm from "../../examples/externalForm";
import CalendarIcon from "../../examples/calendarIcon";
import SelectsMultiple from "../../examples/selectsMultiple";
import CalendarIconExternal from "../../examples/calendarIconExternal";
import CalendarIconSvgIcon from "../../examples/calendarIconSvgIcon";
import ToggleCalendarOnIconClick from "../../examples/toggleCalendarOnIconClick";
Expand Down Expand Up @@ -484,6 +485,10 @@ export default class exampleComponents extends React.Component {
title: "Specific Time Range",
component: ExcludeTimePeriod,
},
{
title: "Select multiple dates",
component: SelectsMultiple,
},
{
title: "Strict parsing",
component: StrictParsing,
Expand Down
15 changes: 15 additions & 0 deletions docs-site/src/examples/selectsMultiple.js
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
Copy link

Choose a reason for hiding this comment

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

Should the example show where to import this from?

🔸 Reduce Future Bugs (Important)

Image of Graham C Graham C

Copy link
Member

Choose a reason for hiding this comment

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

The examples added here are automatically published to https://reactdatepicker.com/

Copy link

Choose a reason for hiding this comment

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

Sure, but how does someone who what line to write in the code to get to the DatePicker component. I am suggesting we add an import line at the top of this example. I guess this is an existing problem with all the examples, so probably out of scope to change.

Image of Graham C Graham C

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I would, but I just copied what the other examples did.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can start by leading with example here and add the import to show how to get it done.

Copy link

Choose a reason for hiding this comment

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

Sounds good to me.

Image of Graham C Graham C

Copy link

Choose a reason for hiding this comment

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

That will fix the automation complaining about DatePicker not being defined...

Image of Andy W Andy W

selectedDates={selectedDates}
selectsMultiple
onChange={onChange}
shouldCloseOnSelect={false}
disabledKeyboardNavigation
/>
);
};
4 changes: 4 additions & 0 deletions src/calendar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ export default class Calendar extends React.Component {
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,
Expand Down Expand Up @@ -920,6 +922,8 @@ export default class Calendar extends React.Component {
selectsEnd={this.props.selectsEnd}
selectsRange={this.props.selectsRange}
selectsDisabledDaysInRange={this.props.selectsDisabledDaysInRange}
selectsMultiple={this.props.selectsMultiple}
selectedDates={this.props.selectedDates}
showWeekNumbers={this.props.showWeekNumbers}
startDate={this.props.startDate}
endDate={this.props.endDate}
Expand Down
17 changes: 17 additions & 0 deletions src/date_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,23 @@
return `${formattedStartDate} - ${formattedEndDate}`;
}

export function safeMultipleDatesFormat(dates, props) {
if (!dates || !dates.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!dates || !dates.length) {
if (!dates?.length) {

return "";

Check warning on line 190 in src/date_utils.js

View check run for this annotation

Codecov / codecov/patch

src/date_utils.js#L190

Added line #L190 was not covered by tests
}
const formattedFirstDate = safeDateFormat(dates[0], props);
if (dates.length === 1) {
return formattedFirstDate;

Check warning on line 194 in src/date_utils.js

View check run for this annotation

Codecov / codecov/patch

src/date_utils.js#L194

Added line #L194 was not covered by tests
}
if (dates.length === 2) {
const formattedSecondDate = safeDateFormat(dates[1], props);
return `${formattedFirstDate}, ${formattedSecondDate}`;
}

const extraDatesCount = dates.length - 1;
return `${formattedFirstDate} (+${extraDatesCount})`;

Check warning on line 202 in src/date_utils.js

View check run for this annotation

Codecov / codecov/patch

src/date_utils.js#L201-L202

Added lines #L201 - L202 were not covered by tests
}

// ** Date Setters **

export function setTime(date, { hour = 0, minute = 0, second = 0 }) {
Expand Down
39 changes: 28 additions & 11 deletions src/day.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -93,14 +95,25 @@

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 = () => {
Copy link

Choose a reason for hiding this comment

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

IMO it would be good to leave a brief comment in the code here describing the intended condition. I think it's pretty hard to parse out why this code is as it is (at least without substantially more context).

🔸 Improve Readability (Important)

Image of Eric E Eric E

if (this.props.disabledKeyboardNavigation) {
return false;
}
if (this.props.selectsMultiple) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @nandorojo 👋
Was digging into this PR to investigate "select multiple months" and I think we need logic similar to this for the months and weeks selection, otherwise there's no selection styles there.
Happy to help with it if I can

const isSelectedDate = (this.props.selectedDates || []).some((date) =>
this.isSameDay(date)

Check warning on line 104 in src/day.jsx

View check run for this annotation

Codecov / codecov/patch

src/day.jsx#L104

Added line #L104 was not covered by tests
);
return !isSelectedDate && this.isSameDay(this.props.preSelection);
} else {
return !(
this.isSameDay(this.props.selected) ||
this.isSameWeek(this.props.selected)
) && (
this.isSameDay(this.props.preSelection) ||
this.isSameWeek(this.props.preSelection)
);
}
Comment on lines +107 to +115
Copy link
Contributor

Choose a reason for hiding this comment

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

the else clause should not be necessary, as the if above ends with a return, so we can skip an indentation here

Suggested change
} else {
return !(
this.isSameDay(this.props.selected) ||
this.isSameWeek(this.props.selected)
) && (
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);

Expand Down Expand Up @@ -275,9 +288,13 @@
};

isCurrentDay = () => this.isSameDay(newDate());

isSelected = () =>
this.isSameDay(this.props.selected) || this.isSameWeek(this.props.selected);

isSelected = () => {
Copy link

Choose a reason for hiding this comment

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

this is the kind of logic update where it would be great to have unit tests. are there any unit tests?

🔸 Improve Test Coverage (Important)

Image of Andy W Andy W

if (this.props.selectsMultiple && this.props.selectedDates) {
return this.props.selectedDates.some((date) => this.isSameDay(date));

Check warning on line 294 in src/day.jsx

View check run for this annotation

Codecov / codecov/patch

src/day.jsx#L294

Added line #L294 was not covered by tests
}
return this.isSameDay(this.props.selected) || this.isSameWeek(this.props.selected);
};
Comment on lines +292 to +297
Copy link
Contributor

@tbowmo tbowmo Feb 25, 2024

Choose a reason for hiding this comment

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

This looks sketchy, as it will fall through to checking on props.selected, if this.props.selectsMultiple is true, and this.props.selectedDates is undefined.

Perhaps something in the line of this instead (I added test on this.isSameWeek() as well, haven't dived enough into the code to see if that actually is necessary?

Suggested change
isSelected = () => {
if (this.props.selectsMultiple && this.props.selectedDates) {
return this.props.selectedDates.some((date) => this.isSameDay(date));
}
return 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(date)) ?? false;
}
return this.isSameDay(this.props.selected) || this.isSameWeek(this.props.selected);
};


getClassNames = (date) => {
const dayClassName = this.props.dayClassName
Expand Down
39 changes: 37 additions & 2 deletions src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
isSameDay,
isMonthDisabled,
isYearDisabled,
safeMultipleDatesFormat,
getHolidaysMap,
isDateBefore,
} from "./date_utils";
Expand Down Expand Up @@ -230,6 +231,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,
Expand Down Expand Up @@ -621,12 +624,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) {
if (
Expand Down Expand Up @@ -667,6 +678,26 @@
if (isRangeFilled) {
onChange([changedDate, null], event);
}
} else if (selectsMultiple) {
const noSelectedDates = !selectedDates || selectedDates.length === 0;

if (noSelectedDates) {
Comment on lines +682 to +684
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to prefer these type of checks here

Suggested change
const noSelectedDates = !selectedDates || selectedDates.length === 0;
if (noSelectedDates) {
if (!selectedDates?.length) {

onChange([changedDate], event);
} else {
const isChangedDateAlreadySelected = selectedDates.some(
(selectedDate) => isSameDay(selectedDate, changedDate)

Check warning on line 688 in src/index.jsx

View check run for this annotation

Codecov / codecov/patch

src/index.jsx#L685-L688

Added lines #L685 - L688 were not covered by tests
);

if (isChangedDateAlreadySelected) {
const nextDates = selectedDates.filter(
(selectedDate) => !isSameDay(selectedDate, changedDate)

Check warning on line 693 in src/index.jsx

View check run for this annotation

Codecov / codecov/patch

src/index.jsx#L692-L693

Added lines #L692 - L693 were not covered by tests
);

onChange(nextDates, event);
} else {
onChange([...selectedDates, changedDate], event);

Check warning on line 698 in src/index.jsx

View check run for this annotation

Codecov / codecov/patch

src/index.jsx#L696-L698

Added lines #L696 - L698 were not covered by tests
}
}
} else {
onChange(changedDate, event);
}
Expand Down Expand Up @@ -1011,6 +1042,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}
Expand Down Expand Up @@ -1194,6 +1227,8 @@
this.props.endDate,
this.props,
)
: this.props.selectsMultiple
? safeMultipleDatesFormat(this.props.selectedDates, this.props)
: safeDateFormat(this.props.selected, this.props);

return React.cloneElement(customInput, {
Expand Down
4 changes: 4 additions & 0 deletions src/month.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ export default class Month extends React.Component {
selectsStart: PropTypes.bool,
selectsRange: PropTypes.bool,
selectsDisabledDaysInRange: PropTypes.bool,
selectsMultiple: PropTypes.bool,
selectedDates: PropTypes.arrayOf(PropTypes.instanceOf(Date)),
showWeekNumbers: PropTypes.bool,
startDate: PropTypes.instanceOf(Date),
setOpen: PropTypes.func,
Expand Down Expand Up @@ -330,6 +332,8 @@ export default class Month extends React.Component {
selectsEnd={this.props.selectsEnd}
selectsRange={this.props.selectsRange}
selectsDisabledDaysInRange={this.props.selectsDisabledDaysInRange}
selectsMultiple={this.props.selectsMultiple}
selectedDates={this.props.selectedDates}
showWeekNumber={this.props.showWeekNumbers}
showWeekPicker={this.props.showWeekPicker}
startDate={this.props.startDate}
Expand Down
4 changes: 4 additions & 0 deletions src/week.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export default class Week extends React.Component {
selectsEnd: PropTypes.bool,
selectsStart: PropTypes.bool,
selectsRange: PropTypes.bool,
selectsMultiple: PropTypes.bool,
selectedDates: PropTypes.arrayOf(PropTypes.instanceOf(Date)),
selectsDisabledDaysInRange: PropTypes.bool,
showWeekNumber: PropTypes.bool,
showWeekPicker: PropTypes.bool,
Expand Down Expand Up @@ -166,6 +168,8 @@ export default class Week extends React.Component {
showWeekPicker={this.props.showWeekPicker}
showWeekNumber={this.props.showWeekNumber}
selectsDisabledDaysInRange={this.props.selectsDisabledDaysInRange}
selectsMultiple={this.props.selectsMultiple}
selectedDates={this.props.selectedDates}
startDate={this.props.startDate}
endDate={this.props.endDate}
dayClassName={this.props.dayClassName}
Expand Down
32 changes: 32 additions & 0 deletions test/multiple_selected_dates.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from "react";
import DatePicker from "../src";
import TestUtils from "react-dom/test-utils";
martijnrusschen marked this conversation as resolved.
Show resolved Hide resolved

describe("Multiple Dates Selected", function () {
function getDatePicker(extraProps) {
return TestUtils.renderIntoDocument(
martijnrusschen marked this conversation as resolved.
Show resolved Hide resolved
<DatePicker
selectsMultiple
onChange={() => {}}
shouldCloseOnSelect={false}
disabledKeyboardNavigation
{...extraProps}
/>,
);
}

it("should handle text format for multiple selections", () => {
martijnrusschen marked this conversation as resolved.
Show resolved Hide resolved
const datePicker = getDatePicker({
selectsMultiple: true,
selectedDates: [new Date("2024/01/01"), new Date("2024/01/15")],
});

expect(datePicker.input.value).toBe("01/01/2024, 01/15/2024");
martijnrusschen marked this conversation as resolved.
Show resolved Hide resolved
});

it("should have multiple highlighted days", () => {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you may have forgotten an expect in this test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was hoping to get some guidance on how to implement this one. I tried copying some helper functions from other tests but haven't been able to figure it out quite yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does anyone with more test / internals knowledge have guidance for these tests? I'm not totally sure what the requirement is

Copy link
Contributor

Choose a reason for hiding this comment

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

@nandorojo As I see it, the displaying of individual selected days should not be tested here.

I have provided a new test that checks if the input text box is rendered properly with 2 and 2+ dates selected.

I have a larger project where this addition could be very useful.

const datePicker = getDatePicker({

Check warning on line 28 in test/multiple_selected_dates.test.js

View workflow job for this annotation

GitHub Actions / update (18.x)

'datePicker' is assigned a value but never used. Allowed unused vars must match /^_/u
selectedDates: [new Date("2024/01/01"), new Date("2024/01/15")],
});
});
martijnrusschen marked this conversation as resolved.
Show resolved Hide resolved
});
Loading