-
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: Screen reader does not announce selected date, current month #3817
Fix: Screen reader does not announce selected date, current month #3817
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3817 +/- ##
==========================================
- Coverage 94.66% 94.59% -0.08%
==========================================
Files 20 20
Lines 1651 1684 +33
Branches 390 404 +14
==========================================
+ Hits 1563 1593 +30
- Misses 25 27 +2
- Partials 63 64 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@martijnrusschen Do I need to add more tests to increase the code-coverage and only then will you review this PR, right? |
}`; | ||
} else { | ||
if (this.props.showTimeSelectOnly) { | ||
ariaLiveMessage = `Selected time: ${safeDateFormat( |
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.
Hardcoded english texts will ruin accessibility for all non-english users. This prefix should be diffferent prop.
Can you resolve the merge conflict so we can get this merged and released? |
…selected-date-announce
@@ -1004,6 +1040,7 @@ export default class Calendar extends React.Component { | |||
showPopperArrow={this.props.showPopperArrow} | |||
arrowProps={this.props.arrowProps} | |||
> | |||
{this.renderAriaLiveRegion()} |
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.
Shouldn't this span be optional?
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.
Agreed, this ariaLiveRegion should be optional
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.
Ah noo, this is already covered here :( #3924
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 agree, this should be optional. This is showing up in my application and I cannot opt out of showing the "Selected date:" text.
…unce Fix: Screen reader does not announce selected date, current month
This PR fixes some accessibility issues, and may help screen reader users navigate this datepicker library better.
I believe that my PR will resolve these issue:
#3558
#3559
#3766