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

Fix #5137: πŸ§ͺβœ… Fix the accidental change of the complete month issue with the test cases by selecting to a earlier date in a month #5138

Merged

Conversation

balajis-qb
Copy link

Closes #5137

Description

I accidentally found this issue with the existing test cases. The issue we're having is with the test module datepicker_test.test.js. There currently 2 test blocks are having an issue as I mentioned in the attached screenshot

  • DatePicker β€Ί should be possible to preSelect minDate (no maxDate set)
  • DatePicker β€Ί should be possible to preSelect minDate (maxDate set)

The Problem
image

I shared one of the failed test cases in the above screenshot. The issue there is we're initially getting a selected day using const selectedDayNode = getSelectedDayNode(data.container) and then doing right and left arrow click over it. This test case will work in most of the days except the last day of a month. Because, let's consider 30th Sept, 2024 as the selected date. When the user press right, a new month view will get rendered and then the existing selectedDayNode will not be available to us anymore. But in our code we didn't refetch the selectedDayNode and do Left click on it. So only the left click didn't get fired and the test case failed.

If the selected date is not the end of the month, this test case will work. Because, eventhough the actual selecteDayNode gets changed to the next date on the initial right arrow click, but still the date we stored in selecteDayNode is still accessible to us and left click over it, will inturn handled by the corresponding component and the test case will pass.

The same issue happens in the other mentioned test case

Possible Fixes:
There are 2 ways to fix the issue

  1. Refetch the selectedDayNode after performing the initial right arrow click and perform left click over it
  2. Update the selectedDate and the minDate to some earlier date in a month, so that the right arrow click won't render a new month view

I fixed the issue in the 2nd way as that requires less code change.

Contribution checklist

  • I have followed the contributing guidelines.
  • I have added sufficient test coverage for my changes.
  • I have formatted my code with Prettier and checked for linting issues with ESLint for code readability.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

βœ… This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:
  • Comments posted to any areas of potential concern or improvement.
  • Detailed feedback or actions needed to resolve issues that are found.
  • Turnaround times vary, but we aim to be swift.

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

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 12
- 3

100% TSX (tests)

Type of change

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

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 96.89%. Comparing base (9ad0ce0) to head (b233974).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5138   +/-   ##
=======================================
  Coverage   96.89%   96.89%           
=======================================
  Files          29       29           
  Lines        3350     3350           
  Branches     1392     1392           
=======================================
  Hits         3246     3246           
  Misses        104      104           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Nice catch. The change looks good to me.

Image of Jacob Jacob


Reviewed with ❀️ by PullRequest

@martijnrusschen martijnrusschen merged commit ce95e64 into Hacker0x01:main Sep 30, 2024
6 checks passed
balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this pull request Oct 6, 2024
- Resolved the issue where navigating to a month with a previously selected date that is now disabled prevents using arrow keys (Tab) to navigate
- Fixed by pre-selecting the first enabled date in the same month.

Closes Hacker0x01#5138
balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this pull request Oct 6, 2024
- Resolved the issue where navigating to a month with a previously selected date that is now disabled prevents using arrow keys (Tab) to navigate
- Fixed by pre-selecting the first enabled date in the same month.

Closes Hacker0x01#5138
balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this pull request Oct 6, 2024
- Resolved the issue where navigating to a month with a previously selected date that is now disabled prevents using arrow keys (Tab) to navigate
- Fixed by pre-selecting the first enabled date in the same month.

Closes Hacker0x01#5138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with the existing Test cases
2 participants