fix: #4360 Cancel Event when clicked/MouseEnter on disabled months/quarters #4607
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4607 +/- ##
==========================================
+ Coverage 96.94% 96.96% +0.01%
==========================================
Files 28 28
Lines 2589 2601 +12
Branches 1091 1095 +4
==========================================
+ Hits 2510 2522 +12
Misses 79 79 ☔ View full report in Codecov by Sentry. |
yuki0410-dev
force-pushed
the
fix/4360
branch
2 times, most recently
from
f4347ba to
5373595
Compare
![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.
@yuki0410-dev you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
As mentioned, this PR is fixing issue number #4360, which reports a subpar UX when clicking on disabled months. The desired behavior is instead to leave the calendar open.
I had one question below about some of the implementation. But otherwise this change seems fine to me.
Reviewed with ❤️ by PullRequest
| ); | ||
| const labelDate = utils.setMonth(this.props.day, m); | ||
|
|
||
| if (utils.isMonthDisabled(labelDate, this.props)) { |
There was a problem hiding this comment.
Are we sure there are no other scenarios where we would not want to disable the mouse enter event for disabled months/quarters?
I can understand the click handlers, but I wonder if we're limiting behavior or potential customization by doing this.
Even though the issue expects all events should be disabled, I'm not sure if that's the best idea unless it's also producing the reported issue. IT seems that just taking care of the click handlers should resolve it.
◽ Clarify Intent
There was a problem hiding this comment.
Thank you for the review.
This behavior is in reference to the processing of clicking on an invalid date in the case of the datepicker.
Line 84 in 87a9835
Line 90 in 87a9835
So far, no issues seem to have been created regarding the date behavior, so I think it is safe to assume that the policy is to match the behavior.
What are your thoughts on this?
There was a problem hiding this comment.
I believe the processing of onMouseEnter is probably a consideration for range selection.
I understand about the loss of customizability, but this is also true for day.jsx, so I will stop at this PR until we can align the behavior of day.jsx and month.jsx.
There was a problem hiding this comment.
for sake of consistency, I think you're doing the right thing by aligning the behaviour with the day.jsx
There was a problem hiding this comment.
I don't have anything further to add beyond the notes already left by Ryan. 🎉
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
PullRequest reviewed the updates made to #4607 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
There was a problem hiding this comment.
PullRequest reviewed the updates made to #4607 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
… months/quarters
Description
Linked issue: close #4360
Problem
See issue #4360
Changes
Fix click/mouseEnter Event Handler iin months/quarters to determine if it is disabled or not.
Screenshots
To reviewers
Contribution checklist