-
Notifications
You must be signed in to change notification settings - Fork 748
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
Improve Coach tabs accessibility for quiz report page #11588
Improve Coach tabs accessibility for quiz report page #11588
Conversation
Build Artifacts
|
Hi @MisRob. I've made the PR targeting the |
Thanks @KshitijThareja, great! I will review some time next week. |
@@ -113,6 +157,12 @@ | |||
} | |||
}, | |||
}, | |||
$trs: { | |||
coachReportsQuizzes: { |
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.
@radinamatic This a string that's used as aria-label
for this tablist
I was thinking if "Coach reports quiz tabs" would make more sense?
Also I think we could improve the string context to say something like "Labels the quiz page tabs in the Coach > Reports > Quizzes"
I'd be grateful to hear from you before making a final suggestion to @KshitijThareja.
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.
Bigger issue here is that since the intention is to merge this into the 0.16 release, we cannot add new strings at this point, as there will be no translations for them available for this release.
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.
Ideally the tablist aria-label should say something like Report details
, but we have nothing similar on Crowdin. Maybe just reusing the string Details
that we do have in our code already, since this tablist is so deep into Coach and reports features that it would be specific enough.
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 okay, I didn't realize this. Thanks @radinamatic. Yes, then let's go with the existing 'Details'
string for now and I will open a follow-up issue for us to replace it with something better. @KshitijThareja The existing string is saved as detailsLabel
defined in commonCoachStrings.js
. Please use it instead of adding a new one. Thank you.
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.
And just as a reminder: when writing strings to use in aria-labels for interactive elements, there is no need to include the type of the element as the screen reader already announces it. For example the tablist on the Coach home has the label Coach reports
, and the NVDA screen reader announces it as:
Coach reports tab control
If you write the string for aria-label Coach reports quiz tabs
as suggested above, NVDA would announce it as:
Coach reports quiz tabs tab control
To sum it up, no need to put the element type inside its aria-label, it is redundant 🙂
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.
I've changed the aria-label
to use the detailsLabel
from commonCoachStrings.js
as asked 👍🏻
Hi @KshitijThareja, thanks, great work. I haven't notice any issues in code and I manually tested it as well. All worked as expected for me. Just leaving one note about a string. I'd like to hear from @radinamatic before we make the final tweak. I think that we can target the release branch if our QA team can test it out, please :)? @radinamatic @pcenov |
Thanks @MisRob :) |
And I missed to confirm that the keyboard navigability on this page is working as intended 🙂 |
Great! @KshitijThareja so the only required update is to re-use the existing string. We will then proceed to merge. Good work! |
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.
Yey for more accessible tab controls in coach pages! 👏🏽
As soon as the label is corrected, this will be good to merge! 💯
Thanks a lot @radinamatic @MisRob 😊 |
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.
Thanks @KshitijThareja and @radinamatic!
Summary
This PR aims to improve the coach tabs accessibility for the quiz report page with the use of KDS tabs (KTabsList and KTabsPanel) in the
QUIZZES
tab which is inside Coach->Reports->Quizzes->Select a quiz. The new constants for the quiz tab are defined intabsConstants.js
.WorkingSample.mp4
References
Closes #10253
Reviewer guidance
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)