-
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
feature: add aria-disabled to Month component #4702
feature: add aria-disabled to Month component #4702
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 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.
@MatteoPieroni 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
+ 23
- 21
66% JavaScript
34% JavaScript (tests)
Type of change
Feature - These changes are adding a new feature or improvement to existing code.
1 Message | |
---|---|
✅ |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4702 +/- ##
=======================================
Coverage 97.09% 97.10%
=======================================
Files 28 28
Lines 2653 2656 +3
Branches 1138 1140 +2
=======================================
+ Hits 2576 2579 +3
Misses 77 77 ☔ 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.
The code seems to match the PR description and includes good test coverage to verify the changes. I didn't see any issues, nice job!
Reviewed with ❤️ by PullRequest
@@ -545,31 +545,27 @@ export default class Month extends React.Component { | |||
} | |||
}; | |||
|
|||
isMonthDisabled = (month) => { |
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.
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 nicely done—straightforward, clean, and well-tested. I didn't notice any issues below, and it's great to see this a finer detail of the ARIA implementation being fixed. Thanks for taking care of this!
Reviewed with ❤️ by PullRequest
Thank you for your reviews and having merged this 🥇 |
Summary
This PR addresses an issue where the Month component is missing the attribute aria-disabled on its disabled items.
Changes made