Fix #5147: 🐛 Fix navigation issue with disabled dates in date picker #5151
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ 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.
There was a problem hiding this comment.
Other than the nit about using safeQuerySelector where appropriate, the change and tests look solid.
Reviewed with ❤️ by PullRequest
src/test/calendar_test.test.tsx
Outdated
| const input = safeQuerySelector<HTMLInputElement>(container, "input"); | ||
| fireEvent.focus(input); | ||
|
|
||
| const nextButton = container.querySelector( |
There was a problem hiding this comment.
ISSUE: @typescript-eslint/no-non-null-assertion (Severity: Low)
Forbidden non-null assertion.
Remediation:
There is safeQuerySelector tucked away in src/test/test-utils.ts which should be preferred in situations like this. It ensures that the element is actually present rather than merely asserting that it is. It's already in scope (used just a couple lines above this), so you should be able to use it without issue.
🤖 powered by PullRequest Automation 👋 verified by Jacob
There was a problem hiding this comment.
There was a problem hiding this comment.
Thank you for the catch. Sorry, I accidentally used it in one test case and copied the same to the other test cases. I fixed it now.
There was a problem hiding this comment.
These changes addressing the navigation issue outlined in #5147 look good to me overall. I don't see any issues with the approach and it's great that enough tests were added to properly simulate what this is addressing.
Reviewed with ❤️ by PullRequest
balajis-qb
force-pushed
the
issue-5147/fix/date-navigation-issue
branch
from
8b57551
to
9fa76c2
Compare
There was a problem hiding this comment.
The latest changes to remove the use of the non-null assertion operator look good to me.
Reviewed with ❤️ by PullRequest
|
The test suite is failing, please check on any issues. |
balajis-qb
force-pushed
the
issue-5147/fix/date-navigation-issue
branch
from
205d9cd
to
abf6f0a
Compare
- 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#5147
balajis-qb
force-pushed
the
issue-5147/fix/date-navigation-issue
branch
from
abf6f0a
to
554671d
Compare
balajis-qb
force-pushed
the
issue-5147/fix/date-navigation-issue
branch
from
554671d
to
c2210dd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5151 +/- ##
==========================================
+ Coverage 96.89% 96.91% +0.01%
==========================================
Files 29 29
Lines 3350 3366 +16
Branches 1405 1396 -9
==========================================
+ Hits 3246 3262 +16
Misses 104 104 ☔ View full report in Codecov by Sentry. |
|
@martijnrusschen, I reinstalled the packages and fixed this code formatting issue. This PR is now safe to be merged. |
There was a problem hiding this comment.
The latest CI checks only indicate existing warnings, so everything seems fine here.
Thanks again for your contribution @balajis-qb! Very appreciated.
Reviewed with ❤️ by PullRequest
Closes #5147
Problem
As I described in the linked ticket, the issue is if we select a date and disable the same date in the next or previous months, then when we navigate to the corresponding month and try using arrow keys (Tab) to navigate to the date, it won't work as there is no pre-selected date available in the month due to the fact that the corresponding date we selected previously is disabled in the corresponding month.
Changes
Contribution checklist