-
Notifications
You must be signed in to change notification settings - Fork 190
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
[BpkCalendar] Accessibility roles #3699
Conversation
Visit https://backpack.github.io/storybook-prs/3699 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3699 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3699 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3699 to see this build running in a browser. |
@@ -193,7 +193,6 @@ class BpkCalendarDate extends PureComponent<Props> { | |||
type="button" | |||
style={style} | |||
className={classNames.join(' ')} | |||
aria-hidden={isBlocked} |
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 think this might be causing the blocked dates in the scrollable calendar to be read by the screen reader which doesn't seem to be happening on the main branch 🤔
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.
Agree with this point, currently the calendar wouldn't allow a AT user to go to dates that are in the past and make them inaccessible so with this removal this now allows them to select previous dates
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 guess are we happy with users being able to get to that date and with it mentioning 'dimmed' this is ok?
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.
Had a chat with Gert about this - it's not possible to keep this with the role of grid as it completely messes it up. Adding grid though brings a lot of benefits though like navigating up and down so even though you can now navigate through the blocked dates, it will be at most 30 days blocked, but you can just move down through the calendar so it's quick
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.
Yes, I purposefully unblocked the dimmed dates as otherwise, Grid navigation wouldn't work correctly :(
BpkCalendar
Add the appropriate 'roles' to different components to make the calendar more accessible and easier to navigate for people using screen readers
You can play test the screen reader and keyboard interaction in the preview
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md