-
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
fix: #4489 Disable weekNumber highlighting when no click handler is assigned. #4608
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4608 +/- ##
=======================================
Coverage 96.94% 96.94%
=======================================
Files 28 28
Lines 2586 2587 +1
Branches 1088 1089 +1
=======================================
+ Hits 2507 2508 +1
Misses 79 79 ☔ View full report in Codecov by Sentry. |
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.
@yuki0410-dev 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
+ 41
- 6
87% JavaScript (tests)
13% JavaScript
Type of change
Fix - These changes are likely to be fixing a bug or issue.
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 PR is addressing issue #4489, which reported an unintuitive UX around week numbers in certain circumstances.
Based on this PR, the core issue appears to be that a clicker handler must be set to enable week number highlighting. The fix is as simple as validating the onClick
props for truthiness before adding the week number react-datepicker__week-number--selected
class name.
It appears that the new behavior will only allow the week number to be rendered as selected if there is an onClick
handler.
This adds several tests around this scenario, which is great. I think these changes look good.
Reviewed with ❤️ by PullRequest
Thank you for the review. I implemented a policy of not highlighting if it is not clickable, because if it is highlighted, the user may think it is clickable. |
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.
Change is simple enough, nice to see some testing for this change too!
Reviewed with ❤️ by PullRequest
…ndler is assigned.
Thanks for the review. |
Description
Linked issue: close #4489
Problem
See issue #4489
Changes
Disable weekNumber highlighting when no click handler is assigned.
Screenshots
To reviewers
Contribution checklist