Select multiple non-consecutive dates #4537

tbowmo · 2024-02-27T07:55:01Z

Rework of #3999 that seems to be stalled.

Added some tests on selected logic for multiple selected dates.
Slightly refactored code to be more readable
for more details see Support selecting multiple dates #3999

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

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

pullrequest

✅ This pull request was sent to the PullRequest network.

@tbowmo you can click here to see the review status or cancel the code review job.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 292
- 14

58% JavaScript (tests)
42% JavaScript

Type of change

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

codecov · 2024-02-27T08:00:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.51%. Comparing base (8089471) to head (4cf8db1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4537      +/-   ##
==========================================
+ Coverage   95.45%   95.51%   +0.05%     
==========================================
  Files          29       29              
  Lines        2533     2566      +33     
  Branches     1039     1053      +14     
==========================================
+ Hits         2418     2451      +33     
  Misses        115      115

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

martijnrusschen · 2024-02-27T08:08:14Z

@tbowmo Thanks for submitting this PR! Are you able to check out the missing coverage that Codecov is complaining about?

tbowmo · 2024-02-27T08:42:21Z

Yup will do.

tbowmo · 2024-02-27T14:38:19Z

@martijnrusschen Hmm.. could we bump codecov? I've updated code here and there, and would be nice to get a new status (I'm used to coverage reports running automatically)

martijnrusschen · 2024-02-27T14:50:07Z

Running it now

martijnrusschen · 2024-02-27T14:53:24Z

Results are updated now

pullrequest

Overall this looks like a clean and well tested implementation. The only thing I'd necessarily want to block on is what seems to be a minor bug in the isSelected function.

Ryan Lester <@buu700>

Reviewed with ❤️ by PullRequest

pullrequest · 2024-02-27T15:37:24Z

docs-site/src/examples/selectsMultiple.js

@@ -0,0 +1,15 @@
+() => {


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>

pullrequest · 2024-02-27T15:37:24Z

docs-site/src/examples/selectsMultiple.js

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


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>

It's the same for all the example files, none of them import DatePicker

👍

Ryan Lester <@buu700>

pullrequest · 2024-02-27T15:37:24Z

src/date_utils.js

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

+export function safeMultipleDatesFormat(dates, props) {


Seems like a lot of care was put into defensive input handling here.

◽ Compliment

Ryan Lester <@buu700>

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

Jacques

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

Also seems that the rest of the safeDateFormat methods is taking care of possible null/undefined inputs

pullrequest · 2024-02-27T15:37:24Z

src/day.jsx

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


This might be slightly more readable as something like:

const isSelectedDate = this.props.selectsMultiple ? this.props.selectedDates?.some( (date) => this.isSameDay(date) || this.isSameWeek(date), ) : this.isSameDay(this.props.selected) || this.isSameWeek(this.props.selected); const isPreSelectedDate = this.isSameDay(this.props.preSelection) || this.isSameWeek(this.props.preSelection); return !isSelectedDate && isPreSelectedDate;

You could also go further and factor out (date) => this.isSameDay(date) || this.isSameWeek(date) to a named isSameDayOrWeek function, which could be used further below in isSelected.

🔹 Code Reuse (Nice to have)

Ryan Lester <@buu700>

pullrequest · 2024-02-27T15:37:24Z

src/day.jsx

+  isSelected = () => {
+    if (this.props.selectsMultiple) {
+      return this.props.selectedDates?.some(
+        (date) => this.isSameDay(date) || this.isSameWeek(this.props.selected),


This looks like a bug: this.isSameWeek(this.props.selected) -> this.isSameWeek(date).

In addition to fixing the bug, consider adding a test to cover this specific issue.

🔺 Bug (Critical)

Ryan Lester <@buu700>

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

oooh.. now I see it.. yup, that should be date and not props.selected :)

Sorry, just to clarify, it seems to me from context that the correct argument to isSameWeek() on this line should be date rather than this.props.selected. (If I misunderstood something, feel free to disregard.)

Ryan Lester <@buu700>

pullrequest · 2024-02-27T15:37:24Z

src/index.jsx


    if (
      !isEqual(this.props.selected, changedDate) ||
      this.props.allowSameDay ||
-      selectsRange
+      selectsRange ||
+      selectsMultiple
    ) {
      if (changedDate !== null) {


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)

Ryan Lester <@buu700>

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

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.

Ryan Lester <@buu700>

porlov · 2024-02-27T17:11:09Z

@tbowmo Does this PR allow to select multiple date ranges?
I think we should support them for consistency too.

tbowmo · 2024-02-27T17:18:26Z

@tbowmo Does this PR allow to select multiple date ranges?
I think we should support them for consistency too.

No, it does not. Date ranges is another set of properties.

I would like to keep this pr as simple as possible, and keep dateranges separated.

Also my current usecase is multiple single dates.

pullrequest

This change looks good and is simple enough

Jacques

Reviewed with ❤️ by PullRequest

pullrequest · 2024-02-27T21:35:28Z

src/day.jsx

+      return false;
+    }
+
+    const isSelectedDate = this.props.selectsMultiple


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

🔹 Improve Readability (Nice to have)

Jacques

pullrequest · 2024-02-27T21:35:28Z

test/multiple_selected_dates.test.js

@@ -0,0 +1,59 @@
+import React from "react";


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

porlov · 2024-02-27T22:30:34Z

No, it does not. Date ranges is another set of properties.

@tbowmo Will it be planned to support it in the future?
Having only multiple dates selection looks inconsistent with base functionality.

pullrequest

Latest changes look good to me!

Ryan Lester <@buu700>

Reviewed with ❤️ by PullRequest

tbowmo · 2024-02-28T07:49:34Z

No, it does not. Date ranges is another set of properties.

@tbowmo Will it be planned to support it in the future? Having only multiple dates selection looks inconsistent with base functionality.

Not sure how it should be implemented at the moment (should we add yet another set of properties for multiple date ranges?).

tbowmo · 2024-02-28T09:24:03Z

There is one test failing, which also fails if I checkout upstream main branch?

martijnrusschen · 2024-02-28T12:17:46Z

Yes it looks like the main branch is failing because of some end of month test that's failing.

martijnrusschen · 2024-02-28T12:29:31Z

Here's a fix #4541 (comment)

martijnrusschen · 2024-02-28T12:45:46Z

Try pulling in the latest main branch

tbowmo · 2024-02-28T13:12:48Z

Yay.. all green now.. Let me know if there are anything that needs to be done further before it's ready to merge

martijnrusschen · 2024-02-28T13:26:14Z

Good to go! Thanks for the effort and help getting this over the finish line.

nandorojo and others added 15 commits April 14, 2023 15:38

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.

Merge branch 'master' into pr/3999

a776678

Merge branch 'main' into master

5052d88

Update package.json

52603c2

remove src

0df17aa

tests

40b5f05

test helpers

a5225ec

Update multiple_selected_dates.test.js

7092c09

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]>

Merge remote-tracking branch 'nandorojo/master' into selectMultipleDates

4d3fbf0

fix comments

7173fb3

Render clear button in multiSelect case

f65ab37

Add coverage

22d608c

pullrequest bot reviewed Feb 27, 2024

View reviewed changes

kmdtqm added 2 commits February 27, 2024 15:23

Simplify logic a bit

e0a7948

enhance coverage

0abfacc

pullrequest bot reviewed Feb 27, 2024

View reviewed changes

Refactoring and code reuse

0a1d636

pullrequest bot reviewed Feb 27, 2024

View reviewed changes

pullrequest bot reviewed Feb 28, 2024

View reviewed changes

Add test for multiselect mode

a148c36

kmdtqm added 2 commits February 28, 2024 10:52

Adding tests for multiselect date handling

ee931c5

Merge remote-tracking branch 'upstream/main' into selectMultipleDates

effa529

Merge remote-tracking branch 'upstream/main' into selectMultipleDates

4cf8db1

martijnrusschen merged commit 698e44e into Hacker0x01:main Feb 28, 2024
6 checks passed

IrvingYHM mentioned this pull request Jun 2, 2024

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

Closed

Select multiple non-consecutive dates #4537

Select multiple non-consecutive dates #4537

Conversation

tbowmo commented Feb 27, 2024 • edited Loading

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

codecov bot commented Feb 27, 2024 • edited Loading

Codecov Report

martijnrusschen commented Feb 27, 2024

tbowmo commented Feb 27, 2024

tbowmo commented Feb 27, 2024

martijnrusschen commented Feb 27, 2024

martijnrusschen commented Feb 27, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

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

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

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

tbowmo Feb 27, 2024

Choose a reason for hiding this comment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

◽ Compliment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

tbowmo Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

tbowmo Feb 28, 2024

Choose a reason for hiding this comment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

🔹 Code Reuse (Nice to have)

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

🔺 Bug (Critical)

tbowmo Feb 27, 2024

Choose a reason for hiding this comment

tbowmo Feb 27, 2024

Choose a reason for hiding this comment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

🔹 Bug (Nice to have)

tbowmo Feb 27, 2024

Choose a reason for hiding this comment

pullrequest bot Feb 28, 2024

Choose a reason for hiding this comment

porlov commented Feb 27, 2024

tbowmo commented Feb 27, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

🔹 Improve Readability (Nice to have)

pullrequest bot Feb 27, 2024

Choose a reason for hiding this comment

🤖 powered by PullRequest Automation 👋 verified by Jacques

porlov commented Feb 27, 2024

pullrequest bot left a comment

Choose a reason for hiding this comment

tbowmo commented Feb 28, 2024

tbowmo commented Feb 28, 2024

martijnrusschen commented Feb 28, 2024

martijnrusschen commented Feb 28, 2024

martijnrusschen commented Feb 28, 2024

tbowmo commented Feb 28, 2024

martijnrusschen commented Feb 28, 2024

tbowmo commented Feb 27, 2024 •

edited

Loading

pullrequest bot left a comment •

edited

Loading

codecov bot commented Feb 27, 2024 •

edited

Loading

tbowmo Feb 28, 2024 •

edited

Loading