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

Select multiple non-consecutive dates #4537

Merged
merged 22 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
() => {
Copy link

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>

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.

ISSUE: react/jsx-no-undef (Severity: Medium)
'DatePicker' is not defined.

Remediation:
Similar to my other comment, not sure if there's a missing import statement for this.

🤖 powered by PullRequest Automation 👋 verified by Ryan Lester <@buu700>

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

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

👍

Image of Ryan Lester <@buu700> Ryan Lester <@buu700>

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 @@ -921,6 +923,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 @@ -197,6 +197,23 @@ export function safeDateRangeFormat(startDate, endDate, props) {
return `${formattedStartDate} - ${formattedEndDate}`;
}

export function safeMultipleDatesFormat(dates, props) {
Copy link

Choose a reason for hiding this comment

The 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.

◽ Compliment

Image of Ryan Lester <@buu700> Ryan Lester <@buu700>

Copy link

Choose a reason for hiding this comment

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

Agreed - is !dates?.length a valid input state? Is "" a value that makes sense for the output of safeMultipleDatesFormat?

Image of Jacques Jacques

Copy link
Contributor Author

@tbowmo tbowmo Feb 28, 2024

Choose a reason for hiding this comment

The 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(', ')
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 }) {
Expand Down
33 changes: 23 additions & 10 deletions src/day.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ export default class Day extends React.Component {
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,17 @@ export default class Day extends React.Component {

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;
}

const isSelectedDate = this.props.selectsMultiple
Copy link

Choose a reason for hiding this comment

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

Good - you can maybe even further break this ball of hair down to one or two more if statements.

🔹 Improve Readability (Nice to have)

Image of Jacques Jacques

? this.props.selectedDates?.some((date) => this.isSameDayOrWeek(date))
: this.isSameDayOrWeek(this.props.selected);

return !isSelectedDate && this.isSameDayOrWeek(this.props.preSelection);
};

isDisabled = () => isDayDisabled(this.props.day, this.props);

Expand All @@ -127,6 +132,8 @@ export default class Day extends React.Component {
),
);

isSameDayOrWeek = (other) => this.isSameDay(other) || this.isSameWeek(other);

getHighLightedClass = () => {
const { day, highlightDates } = this.props;

Expand Down Expand Up @@ -276,8 +283,14 @@ export default class Day extends React.Component {

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.isSameDayOrWeek(date),
);
}
return this.isSameDayOrWeek(this.props.selected);
};

getClassNames = (date) => {
const dayClassName = this.props.dayClassName
Expand Down
45 changes: 41 additions & 4 deletions src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
isSameDay,
isMonthDisabled,
isYearDisabled,
safeMultipleDatesFormat,
getHolidaysMap,
isDateBefore,
} from "./date_utils";
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Copy link

Choose a reason for hiding this comment

The 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 changedDate might be undefined? If so, consider changing this to != null (as used in code further below).

🔹 Bug (Nice to have)

Image of Ryan Lester <@buu700> Ryan Lester <@buu700>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this one? haven't touched this part myself..

Copy link

Choose a reason for hiding this comment

The 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 changedDate was. Seems out of scope of this PR either way, but may be worth someone taking a look at separately.

Image of Ryan Lester <@buu700> Ryan Lester <@buu700>

if (
Expand Down Expand Up @@ -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),

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 @@ -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}
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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
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 @@ -331,6 +333,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
29 changes: 29 additions & 0 deletions test/day_test.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,14 @@ describe("Day", () => {
.getAttribute("aria-selected");
expect(ariaSelected).toBe("true");
});

it("should apply the selected class for selectedDates", () => {
const shallowDay = renderDay(day, {
selectsMultiple: true,
selectedDates: [day],
});
expect(shallowDay.hasClass(className)).toBe(true);
});
});

describe("if not selected", () => {
Expand Down Expand Up @@ -115,12 +123,33 @@ describe("Day", () => {
expect(shallowDay.hasClass(className)).toBe(true);
});

it("should apply the keyboard-selected class when pre-selected and another days is multi-selected", () => {
const day = newDate();
const selected = addDays(day, 1);
const shallowDay = renderDay(day, {
selectedDates: [selected],
selectsMultiple: true,
preSelection: day,
});
expect(shallowDay.hasClass(className)).toBe(true);
});

it("should not apply the keyboard-selected class when selected", () => {
const day = newDate();
const shallowDay = renderDay(day, { selected: day, preSelection: day });
expect(shallowDay.hasClass(className)).toBe(false);
});

it("should not apply the keyboard-selected class when selected a multi-selected day", () => {
const day = newDate();
const shallowDay = renderDay(day, {
selectedDates: [day],
selectsMultiple: true,
preSelection: day,
});
expect(shallowDay.hasClass(className)).toBe(false);
});

it("should not apply the keyboard-selected class when another day is pre-selected", () => {
const day = newDate();
const selected = addDays(day, 1);
Expand Down
59 changes: 59 additions & 0 deletions test/multiple_selected_dates.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import React from "react";
Copy link

Choose a reason for hiding this comment

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

ISSUE: no-unused-vars (Severity: Low)
'React' is defined but never used.

Remediation:
Pointing this out just in case it's not actually needed

🤖 powered by PullRequest Automation 👋 verified by Jacques

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)");
});
});
Loading