-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Update showQuarterYearPicker to respect disableKeyboardNavigation #4356
Conversation
There was a problem hiding this 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.
@linuxlewis you can click here to see the review status or cancel the code review job.
There was a problem hiding this 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
+ 25
- 1
88% JavaScript (tests)
12% JavaScript
Type of change
Minor Update - These changes appear to be a minor update to existing functionality and features.
1 Message | |
---|---|
📚 | It looks like the description for this pull request is either blank or very short. Adding a high-level summary will help our reviewers provide better feedback. Feel free to include questions for PullRequest reviewers and make specific feedback requests. |
Codecov Report
@@ Coverage Diff @@
## main #4356 +/- ##
=======================================
Coverage 96.67% 96.67%
=======================================
Files 27 27
Lines 2373 2374 +1
Branches 965 966 +1
=======================================
+ Hits 2294 2295 +1
Misses 79 79
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change should work expected (thanks for adding tests), but I'm not sure why this change is needed. Won't disabling keyboard navigation pose a problem for accessibility?
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly, this option to disable keyboard navigation exists already and this is a fix for a UI bug. Based off of that assumption, the code looks good to me and validated through the test that was added.
Reviewed with ❤️ by PullRequest
No description provided.