🐛🚀 Fix and Enhance Date Range Selection #4306

franmc01 · 2023-10-11T06:32:43Z

Steps to Reproduce 🕵️‍♂️

Visit the documentation and date range selection example:
Navigate to the date range picker example in the documentation.
Observe the second input (for endDate):
Note that the days prior to the date selected in the first input are disabled, which may not be the desired behavior in some use cases.
Select a start date (startDate) that is after the end date (endDate):
The invalid range is allowed, posing a usability concern as users can select a start date that is after their chosen end date, which might not be intuitive or may not validate against business logic.

🚨 Issue:

📆 Invalid Date Ranges: The component allows users to select invalid date ranges when using two inputs for range selection.
🚫 Days Becoming Incorrectly Disabled: Selecting certain date ranges results in days becoming incorrectly disabled in the UI.

💡 Proposed Solutions:

🔒 Prevent Invalid Range Selection: Implement logic to prevent the user from selecting an invalid date range by doing nothing (or potentially showing an error message) when they attempt to select an end date that is earlier than the start date and vice versa.

if (selectsStart || selectsEnd) {
  if (isValid(new Date(startDate)) && isValid(new Date(endDate))) {
    if (isAfter(new Date(changedDate), new Date(endDate)) && selectsStart) {
      return;
    } else if (isBefore(new Date(changedDate), new Date(startDate)) && selectsEnd) {
      return;
    }
  }
}

🔄 Equalizing Start and End Dates: Introduce logic that sets the end date equal to the start date if the user attempts to set a start date that is later than the end date, and vice versa.
🔄 Resetting the Opposite Input: If a user selects an invalid range, automatically reset the opposite input (reset end date if an invalid start date is selected, and vice versa).
✨ Allow Invalid Ranges Prop: Introduce a new prop allowInvalidRanges, which enables developers to control whether invalid date ranges can be set by the user. When true, invalid ranges are allowed, potentially enabling developers to handle this case in a way that suits their application, such as displaying a custom error message.
```
<DatePicker
  selected={startDate}
  onChange={(date) => setStartDate(date)}
  selectsStart
  startDate={startDate}
  endDate={endDate}
  allowInvalidRanges={false}
/>
```

📝 Additional Notes:

Developers must be able to select an approach that suits their application’s needs.
The "days becoming incorrectly disabled" issue needs to be addressed to ensure user experience remains consistent and logical.

📋 Tasks:

Implement logic to prevent invalid date range selection.
Ensure days do not become incorrectly disabled.
Implement logic to equalize start and end dates upon invalid selection.
Implement logic to reset opposite input upon invalid selection.
Implement allowInvalidRanges prop to provide additional flexibility for developers.
Update documentation and examples to illustrate the usage of the new prop.

🧪 Tests:

Ensure all existing tests pass.
Add new tests for the introduced logic and prop to ensure future changes do not break this functionality.

NOTE: Temporarily disable linter rules in tests due to React 18 updates

- Add logic to disallow the selection of an end date before the start date and vice versa in the date range picker. - Temporarily disable linter rules in tests due to React 18 updates and

pullrequest

✅ This pull request was sent to the PullRequest network.

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

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 27
- 1

93% JavaScript
7% JavaScript (tests)

Type of change

Fix - These changes are likely to be fixing a bug or issue.

codecov · 2023-10-11T06:34:37Z

Codecov Report

Merging #4306 (bb25295) into main (d770de8) will decrease coverage by 0.37%.
Report is 6 commits behind head on main.
The diff coverage is 18.18%.

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

@@            Coverage Diff             @@
##             main    #4306      +/-   ##
==========================================
- Coverage   96.54%   96.18%   -0.37%     
==========================================
  Files          27       27              
  Lines        2374     2384      +10     
  Branches      966      974       +8     
==========================================
+ Hits         2292     2293       +1     
- Misses         82       91       +9

Files	Coverage Δ
src/index.jsx	`92.48% <18.18%> (-1.76%)`	⬇️

pullrequest

I have a concern left inline r.e. the catch block. We're also missing test coverage for the changes here.

Graham C

Reviewed with ❤️ by PullRequest

pullrequest · 2023-10-11T08:43:28Z

src/index.jsx

+        this.setState({ inputValue: null });
+      }
+    } catch (error) {
+      console.log(error);


Is catching the error actually the appropriate thing to do? Logging using console.log is perhaps also not appropriate. Either we should throw this error so the caller can see it, or at least use console.error. Entering this catch is also missing test coverage.

🔺 Bug (Critical)

Graham C

I agree that a more appropriate log level would be better here. However that ties in with my point on understanding what errors are actually going to occur here.

Ryan

👋 Hello everyone!

I would like to provide some clarifications regarding the use of the try...catch block in the previously shared code. 🙇‍♂️

Initially, I introduced this block in an attempt to pinpoint the invalid date issue that I was grappling with during the debugging phase. The idea was to ensure that, if any related error emerged, I could rapidly identify its origin. 🕵️‍♂️

However, upon further review and additional testing, I was able to verify that that particular section of the code was not the root cause of the issue at hand, thus rendering the try...catch not only unnecessary but also disruptive to code fluidity and readability. 🧪🔍

Consequently, I have now removed this block from the code to preserve clarity and prevent confusion in future reviews. 🧹✨

I greatly appreciate your attention and sharp observation. Any feedback is always valuable! Moreover, have you had a chance to observe the error mentioned in the PR? Gaining insights from your impressions and awareness of this issue would be immensely helpful. 🚀

pullrequest

Graham has already left some good feedback inline. I had an additional concern about the broad try-catch block without a clear picture of what errors we'll actually encounter.

Ryan

Reviewed with ❤️ by PullRequest

src/index.jsx

Eliminate the try...catch block that was initially introduced for debugging the invalid date issue, but was determined not to be the root cause and therefore is not needed.

pullrequest

We're still missing test coverage. I have another comment left inline, too.

Graham C

Reviewed with ❤️ by PullRequest

pullrequest · 2023-10-11T14:51:48Z

src/index.jsx

@@ -639,6 +646,23 @@ export default class DatePicker extends React.Component {
          this.setState({ monthSelectedIn: monthSelectedIn });
        }
      }
+
+      if (selectsStart || selectsEnd) {


Can we cleanup the logic here? The extra nesting and else ifs are unnecessary.

🔸 Simplify Code (Important)

Graham C

Why? Could you explain me because are unnecessary?

The level of nesting and use of else. One can replace lines 650-664 with:

if ((selectsStart || selectsEnd) && isValid(new Date(startDate)) && isValid(new Date(endDate))) { if (isAfter(new Date(changedDate), new Date(endDate)) && selectsStart) { return; } if (isBefore(new Date(changedDate), new Date(startDate)) && selectsEnd) { return; } }

Graham C

martijnrusschen · 2023-10-11T18:34:59Z

test/timepicker_test.test.js

@@ -1,3 +1,4 @@
+/* eslint-disable react/no-deprecated */


Why is this needed?

@martijnrusschen I wrote: Temporarily disable linter rules in tests due to React 18 updates

martijnrusschen · 2023-10-11T18:35:05Z

test/datepicker_test.test.js

@@ -1,3 +1,4 @@
+/* eslint-disable react/no-deprecated */


Why is this needed?

pullrequest

I agree that more tests could be added to capture the fix and new behavior. At a minimum a test for the original bug would be good to prevent future regressions.

Ryan

Reviewed with ❤️ by PullRequest

feat(date-range): 🚫 Prevent Invalid Range Selection

8790def

- Add logic to disallow the selection of an end date before the start date and vice versa in the date range picker. - Temporarily disable linter rules in tests due to React 18 updates and

pullrequest bot reviewed Oct 11, 2023

View reviewed changes

src/index.jsx Outdated Show resolved Hide resolved

Remove unnecessary try...catch block

104977f

Eliminate the try...catch block that was initially introduced for debugging the invalid date issue, but was determined not to be the root cause and therefore is not needed.

pullrequest bot reviewed Oct 11, 2023

View reviewed changes

martijnrusschen reviewed Oct 11, 2023

View reviewed changes

pullrequest bot reviewed Oct 12, 2023

View reviewed changes

franmc01 closed this Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛🚀 Fix and Enhance Date Range Selection #4306

🐛🚀 Fix and Enhance Date Range Selection #4306

franmc01 commented Oct 11, 2023 •

edited

Loading

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

pullrequest bot left a comment

pullrequest bot Oct 11, 2023

pullrequest bot Oct 11, 2023

franmc01 Oct 11, 2023

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot Oct 11, 2023

franmc01 Oct 14, 2023

pullrequest bot Oct 15, 2023

martijnrusschen Oct 11, 2023

franmc01 Oct 14, 2023

martijnrusschen Oct 11, 2023

pullrequest bot left a comment

🐛🚀 Fix and Enhance Date Range Selection #4306

🐛🚀 Fix and Enhance Date Range Selection #4306

Conversation

franmc01 commented Oct 11, 2023 • edited Loading

Steps to Reproduce 🕵️‍♂️

🚨 Issue:

💡 Proposed Solutions:

📝 Additional Notes:

📋 Tasks:

🧪 Tests:

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 Oct 11, 2023 • edited Loading

Codecov Report

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Oct 11, 2023

Choose a reason for hiding this comment

🔺 Bug (Critical)

pullrequest bot Oct 11, 2023

Choose a reason for hiding this comment

franmc01 Oct 11, 2023

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

pullrequest bot Oct 11, 2023

Choose a reason for hiding this comment

🔸 Simplify Code (Important)

franmc01 Oct 14, 2023

Choose a reason for hiding this comment

pullrequest bot Oct 15, 2023

Choose a reason for hiding this comment

martijnrusschen Oct 11, 2023

Choose a reason for hiding this comment

franmc01 Oct 14, 2023

Choose a reason for hiding this comment

martijnrusschen Oct 11, 2023

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

franmc01 commented Oct 11, 2023 •

edited

Loading

pullrequest bot left a comment •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading