-
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: #4401 monthsShown with MonthYearPicker or QuarterYearPicker #4603
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.
@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
+ 112
- 9
78% JavaScript (tests)
22% JavaScript
Type of change
Fix - These changes are likely to be fixing a bug or issue.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4603 +/- ##
=======================================
Coverage 96.94% 96.94%
=======================================
Files 28 28
Lines 2582 2586 +4
Branches 1081 1088 +7
=======================================
+ Hits 2503 2507 +4
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.
These changes look good for resolving this issue. Given that it is performing some changes to the calendar behavior, I'd feel more confident in these changes if they had a solid set of unit tests with them.
One example would be—considering this was a bug, then there should be a unit test verifying that this works as expected now. This would protect against a regression if this code changes in the future. This particular test might look like: "it does not display the same year in both calendars for the month range selector".
On top of that, I'd recommend adding tests for the new behavior here too and thinking through any possible edge cases. For example, verifying that the year on the left-hand side is always less than the year on the right-hand side.
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.
Tests were added. |
Description
Linked issue: close #4401
Problem
See issue #4401
Changes
XX--keyboard-selected
class.Screenshots
To reviewers
Contribution checklist