Support selecting multiple dates #3999

nandorojo · 2023-04-14T19:45:37Z

React.Datepicker.crafted.by.HackerOne.-.14.April.2023.mp4

What

This PR adds 2 props which allow users to select multiple isolated dates on the calendar picker.

You can now select multiple dates at once, which is very convenient for products that need users to select multiple dates, such as Calendly and the like.

How

Following the API for selectsRange, this PR adds a selectsMultiple boolean, in addition to a selectedDates array.

I thought it was best for selectedDates to be explicitly different from selected. This is also following the convention from selectsRange, which uses startDate and endDate.

Example

As shown in the video above, here is the example code:

() => {
  const [selectedDates, setSelectedDates] = useState([new Date()]);
  const onChange = (dates) => {
    setSelectedDates(dates);
  }
  return (
    <DatePicker
      selectedDates={selectedDates}
      selectsMultiple
      onChange={onChange}
      shouldCloseOnSelect={false}
      disabledKeyboardNavigation
    />
  )
}

Breakdown

This PR adds a few minor changes to achieve adding multiple dates.

First, in the Picker component, it adds 2 core changes:

Formatting the input with safeMultipleDatesFormat.
Properly accounting for selecting multiple dates in onChange.

The rest is just prop drilling down to <Day />, which has 2 key changes:

isKeyboardSelected needs to account for selectedDates rather than selected.
isSelected also checks for selectedDates

I've tested this out and it all looks good to me!

…>> This PR adds 2 props which allow users to select multiple isolated dates on the calendar picker. It adds changes to Picker.onChange(), Day.isSelected(), and adds formatting for the input.

codecov · 2023-05-24T17:24:43Z

Codecov Report

Attention: Patch coverage is 56.52174% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 94.74%. Comparing base (3d8e0cb) to head (a5225ec).
Report is 104 commits behind head on main.

❗ Current head a5225ec differs from pull request most recent head 3151397. Consider uploading reports for the commit 3151397 to get more accurate results

Files	Patch %	Lines
src/index.jsx	33.33%	12 Missing ⚠️
src/date_utils.js	63.63%	4 Missing ⚠️
src/day.jsx	76.47%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3999      +/-   ##
==========================================
- Coverage   95.46%   94.74%   -0.73%     
==========================================
  Files          29       29              
  Lines        2513     2548      +35     
  Branches     1026     1043      +17     
==========================================
+ Hits         2399     2414      +15     
- Misses        114      134      +20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pullrequest

✅ This pull request was sent to the PullRequest network.

@nandorojo you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 146
- 13

80% JavaScript
20% JavaScript (tests)

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

pullrequest

This is looking like a great start. The code is looking clean, but has no test coverage. I have only one minor note left inline for your consideration.

Graham C

Reviewed with ❤️ by PullRequest

pullrequest · 2023-06-30T14:17:29Z

docs-site/src/examples/selectsMultiple.js

+    setSelectedDates(dates);
+  };
+  return (
+    <DatePicker


Should the example show where to import this from?

🔸 Reduce Future Bugs (Important)

Graham C

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

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.

Graham C

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

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

Sounds good to me.

Graham C

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

Andy W

martijnrusschen · 2023-06-30T14:23:22Z

@nandorojo Can you add some tests to cover your new functionality?

nandorojo · 2023-06-30T14:23:58Z

Sure, other than the example app updates?

martijnrusschen · 2023-06-30T14:25:17Z

Ideally, add some automated tests to ensure we don't break this in future changes. You can see the tests folder with examples of how to test.

nandorojo · 2023-06-30T14:27:48Z

Got it, I can do that today if it means getting it merged. Thanks for the feedback.

pullrequest

This looks to support multiple days as described but given the new functionality it would be great to see some tests and documentation around the new feature

James D

Reviewed with ❤️ by PullRequest

danMateer · 2023-07-03T21:26:53Z

James D and Graham C from PullRequest - any test cases you can recommend to @nandorojo just so he can knock them out?

I'm 100% sure he's fully capable, just very busy juggling a lot 1 2. It helps sometimes to just have stuff listed in GitHub even if it's basic and obvious.

pullrequest

For adding tests:

if there isn't already one, I would add a test around using this component to select a single day
Using this component to select multiple days
Using this component to select a single day and then change it
Using this component to select multiple days and then change one of them

It appears those are supported scenarios that would be useful to add here for coverage

James D

Reviewed with ❤️ by PullRequest

pullrequest

I agree with the suggestions from James.

Graham C

Reviewed with ❤️ by PullRequest

martijnrusschen

Pending test coverage

pullrequest

PullRequest reviewed the updates made to #3999 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Graham C

pullrequest

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.

garrettparris · 2023-08-22T01:16:00Z

any way to just merge this? useful feature and the code looks fine.

Zarthus · 2023-09-11T09:19:10Z

any way to just merge this? useful feature and the code looks fine.

@garrettparris @williamtcastro @nandorojo

Seems like it's currently blocked on some missing tests, we recently migrated to jest so hopefully it's easier to add tests now. There's also a merge conflict error.

nandorojo · 2024-01-08T16:50:02Z

Hey guys. Is this worth me going into and fixing merge conflicts? I can do it, but I don't want to spend the time again if it won't get merged.

We used the code in prod for months with no issues. However, I just upgraded to 4.14.0 and lost all my changes, so I'm hoping to use this PR now. Please let me know if this is worth doing or not.

nandorojo · 2024-01-08T16:53:08Z

Merge conflicts fixed.

nandorojo · 2024-01-08T16:53:49Z

Not sure where tests are failing, I don't see them on here.

martijnrusschen · 2024-01-08T16:55:43Z

@nandorojo I've started the workflows for test coverage. You should see those showing up in a minute. Per request, can you add some test coverage for this functionality? I'm happy to get this merged once ready.

martijnrusschen · 2024-01-08T16:56:41Z

Per workflow results, the codecov check is complaining about missing test coverage.

pullrequest

✅ This pull request was sent to the PullRequest network.

@nandorojo you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

I love this component and this looks like a great update. It seems like there are some rather complicated logic improvements that could benefit from unit test coverage, though.

Andy W

Reviewed with ❤️ by PullRequest

pullrequest · 2024-01-08T17:18:00Z

src/day.jsx

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


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)

Andy W

pullrequest

This multi-selection functionality looks good overall. I just had the one comment suggesting that a code comment be left for readability in one spot that was pretty non-obvious. Other than that, I don't have anything that hasn't already been mentioned by other reviewers.

Eric E

Reviewed with ❤️ by PullRequest

pullrequest · 2024-01-08T17:33:53Z

src/day.jsx

-    ) &&
-    (this.isSameDay(this.props.preSelection) ||
-      this.isSameWeek(this.props.preSelection));
+  isKeyboardSelected = () => {


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)

Eric E

martijnrusschen · 2024-01-08T19:38:44Z

test/multiple_selected_dates.test.js

+    expect(datePicker.input.value).toBe("01/01/2024, 01/15/2024");
+  });
+
+  it("should have multiple highlighted days", () => {


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

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.

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

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

pullrequest

PullRequest reviewed the updates made to #3999 up until the latest commit (a5225ec). No further issues were found.

Reviewed by:

Andy W

pullrequest

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.

tbowmo · 2024-02-19T11:22:37Z

@martijnrusschen @nandorojo
is it only test that is keeping this from being merged? If so, is there anything we could help with in this regard? (Multiple dates could be really useful for my project)

test/multiple_selected_dates.test.js

Co-authored-by: Thomas Mørch <[email protected]>

martijnrusschen · 2024-02-25T18:25:39Z

@nandorojo can you resolve the merge conflicts?

tbowmo

Found a couple of minor issues (perhaps some is just nitpicking),

Not entirely sure about code coverage or how I can help with adding that without creating a new PR against @nandorojo s branch (if needed)

tbowmo · 2024-02-25T19:47:27Z

src/index.jsx

+        const noSelectedDates = !selectedDates || selectedDates.length === 0;
+
+        if (noSelectedDates) {


I tend to prefer these type of checks here

Suggested change

const noSelectedDates = !selectedDates || selectedDates.length === 0;

if (noSelectedDates) {

if (!selectedDates?.length) {

tbowmo · 2024-02-25T19:53:53Z

src/day.jsx

+    } else {
+      return !(
+        this.isSameDay(this.props.selected) ||
+        this.isSameWeek(this.props.selected)
+      ) && (
+        this.isSameDay(this.props.preSelection) ||
+        this.isSameWeek(this.props.preSelection)
+      );
+    }


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)

);

tbowmo · 2024-02-25T19:58:32Z

src/day.jsx

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


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

};

tbowmo · 2024-02-25T20:00:25Z

src/date_utils.js

@@ -185,6 +185,23 @@ export function safeDateRangeFormat(startDate, endDate, props) {
  return `${formattedStartDate} - ${formattedEndDate}`;
 }

+export function safeMultipleDatesFormat(dates, props) {
+  if (!dates || !dates.length) {


Suggested change

if (!dates || !dates.length) {

if (!dates?.length) {

martijnrusschen · 2024-02-25T20:16:21Z

@tbowmo Thanks for the suggestions. It would be great to get this PR over the finish line. If it's easier, perhaps we should consider opening a new PR with your changes included. Let me know if you prefer that and I can review the changes once its in.

tbowmo · 2024-02-25T21:22:21Z

So you suggest that I fork @nandorojo working branch, add my suggestions, and then create a new pr? (perhaps add a few tests when I get it figured out)

luistorres · 2024-06-27T23:07:12Z

src/day.jsx

+    if (this.props.disabledKeyboardNavigation) {
+      return false;
+    }
+    if (this.props.selectsMultiple) {


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

Support selecting multiple dates via selectsMultiple & selectedDates …

b2cca19

…>> This PR adds 2 props which allow users to select multiple isolated dates on the calendar picker. It adds changes to Picker.onChange(), Day.isSelected(), and adds formatting for the input.

nandorojo mentioned this pull request Apr 14, 2023

Add multiple dates functionallity #3981

Open

Merge branch 'master' into pr/3999

a776678

pullrequest bot reviewed Jun 30, 2023

View reviewed changes

pullrequest bot reviewed Jul 1, 2023

View reviewed changes

pullrequest bot reviewed Jul 5, 2023

View reviewed changes

martijnrusschen requested changes Jul 10, 2023

View reviewed changes

pullrequest bot reviewed Jul 10, 2023

View reviewed changes

pullrequest bot reviewed Jul 24, 2023

View reviewed changes

williamtcastro approved these changes Sep 11, 2023

View reviewed changes

Merge branch 'main' into master

5052d88

pullrequest bot reviewed Jan 8, 2024

View reviewed changes

nandorojo added 2 commits January 8, 2024 14:02

Update package.json

52603c2

remove src

0df17aa

pullrequest bot reviewed Jan 8, 2024

View reviewed changes

nandorojo added 2 commits January 8, 2024 16:27

tests

40b5f05

test helpers

a5225ec

martijnrusschen reviewed Jan 8, 2024

View reviewed changes

pullrequest bot reviewed Jan 8, 2024

View reviewed changes

pullrequest bot reviewed Jan 24, 2024

View reviewed changes

tbowmo suggested changes Feb 19, 2024

View reviewed changes

test/multiple_selected_dates.test.js Outdated Show resolved Hide resolved

test/multiple_selected_dates.test.js Show resolved Hide resolved

Update multiple_selected_dates.test.js

7092c09

tbowmo suggested changes Feb 25, 2024

View reviewed changes

test/multiple_selected_dates.test.js Outdated Show resolved Hide resolved

test/multiple_selected_dates.test.js Outdated Show resolved Hide resolved

test/multiple_selected_dates.test.js Outdated Show resolved Hide resolved

martijnrusschen and others added 3 commits February 25, 2024 19:23

Update test/multiple_selected_dates.test.js

9593bcb

Co-authored-by: Thomas Mørch <[email protected]>

Update test/multiple_selected_dates.test.js

f6e2d80

Co-authored-by: Thomas Mørch <[email protected]>

Update test/multiple_selected_dates.test.js

3151397

Co-authored-by: Thomas Mørch <[email protected]>

tbowmo reviewed Feb 25, 2024

View reviewed changes

tbowmo mentioned this pull request Feb 27, 2024

Select multiple non-consecutive dates #4537

Merged

martijnrusschen merged commit 4d3fbf0 into Hacker0x01:main Feb 28, 2024

IrvingYHM mentioned this pull request Jun 2, 2024

[Snyk] Upgrade react-datepicker from 6.2.0 to 6.9.0 IrvingYHM/Opticenter#26

Closed

luistorres reviewed Jun 27, 2024

View reviewed changes

		const noSelectedDates = !selectedDates \|\| selectedDates.length === 0;

		if (noSelectedDates) {

Support selecting multiple dates #3999

Support selecting multiple dates #3999

Conversation

nandorojo commented Apr 14, 2023 • edited Loading

What

How

Example

Breakdown

codecov bot commented May 24, 2023 • edited Loading

Codecov Report

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

PullRequest Breakdown

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Jun 30, 2023

Choose a reason for hiding this comment

🔸 Reduce Future Bugs (Important)

Choose a reason for hiding this comment

pullrequest bot Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Jun 30, 2023

Choose a reason for hiding this comment

pullrequest bot Jan 9, 2024

Choose a reason for hiding this comment

martijnrusschen commented Jun 30, 2023

nandorojo commented Jun 30, 2023

martijnrusschen commented Jun 30, 2023

nandorojo commented Jun 30, 2023

pullrequest bot left a comment

Choose a reason for hiding this comment

danMateer commented Jul 3, 2023

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

martijnrusschen left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

garrettparris commented Aug 22, 2023

Zarthus commented Sep 11, 2023

nandorojo commented Jan 8, 2024 • edited Loading

nandorojo commented Jan 8, 2024

nandorojo commented Jan 8, 2024

martijnrusschen commented Jan 8, 2024

martijnrusschen commented Jan 8, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Jan 8, 2024

Choose a reason for hiding this comment

🔸 Improve Test Coverage (Important)

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Jan 8, 2024

Choose a reason for hiding this comment

🔸 Improve Readability (Important)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

tbowmo commented Feb 19, 2024

martijnrusschen commented Feb 25, 2024

tbowmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbowmo Feb 25, 2024 • edited Loading

nandorojo commented Apr 14, 2023 •

edited

Loading

codecov bot commented May 24, 2023 •

edited

Loading

pullrequest bot left a comment •

edited

Loading

nandorojo commented Jan 8, 2024 •

edited

Loading

tbowmo Feb 25, 2024 •

edited

Loading