New: Required and optional labels added (Fixes #191) #193
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,6 +27,14 @@ | |||||
| </div> | ||||||
| {{/if}} | ||||||
|
|
||||||
| {{#all _globals._accessibility._ariaLabels.optional _globals._accessibility._ariaLabels.required}} | ||||||
| <div class='menu-item__priority boxmenu-item__priority' aria-hidden="true"> | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| <span class="label"> | ||||||
| {{#if _isOptional}}{{{_globals._accessibility._ariaLabels.optional}}}{{else}}{{{_globals._accessibility._ariaLabels.required}}}{{/if}} | ||||||
| </span> | ||||||
| </div> | ||||||
| {{/all}} | ||||||
|
|
||||||
| {{#if duration}} | ||||||
| <div class="menu-item__duration boxmenu-item__duration"> | ||||||
| <div class="menu-item__duration-inner boxmenu-item__duration-inner"> | ||||||
|
|
@@ -40,7 +48,7 @@ | |||||
| </div> | ||||||
|
|
||||||
| <div class="menu-item__button-container boxmenu-item__button-container"> | ||||||
| <button class="btn-text menu-item__button boxmenu-item__button js-btn-click{{#if _isVisited}} is-visited{{/if}}{{#if _isLocked}} is-locked{{/if}}" aria-label="{{#if _isComplete}}{{_globals._accessibility._ariaLabels.complete}}. {{else _isVisited}}{{_globals._accessibility._ariaLabels.visited}}. {{/if}}{{#if _isLocked}}{{_globals._accessibility._ariaLabels.locked}}. {{else}}{{linkText}} {{/if}}{{title}}. {{{compile _globals._menu._boxMenu.itemCount}}}.{{#if _isOptional}} {{_globals._accessibility._ariaLabels.optional}}{{else}} {{_globals._accessibility._ariaLabels.required}}{{/if}}"{{#if _isLocked}} aria-disabled="true"{{/if}} role="link"> | ||||||
|
There was a problem hiding this comment. Is there a rationale for this? Just an issue that says "I'm intending to add required and optional text to X places for these reasons?". This issue is the nearest adaptlearning/adapt-contrib-core#520, but there are no designs on how it will look and where it will be placed. Currently optional articles, blocks and components don't display as such except when included in the plp by saying "optional content". Could you supply a bit more of the thought please? Perhaps in an encompassing issue in adapt_framework? I like this work. It's a bit like the completion indicators we've been slowly but collectively adding throughout. It sets up nice uniform UX for the learners. There was a problem hiding this comment. yep, we do need a discussion around what happens with optional content thoughtout the course and in the PLP in different situations. I'll try and raise a succinct separate issue for this and add a link in here. That should help answer the questions raised by @oliverfoster |
||||||
| {{{linkText}}} | ||||||
| </button> | ||||||
|
|
||||||
|
|
||||||
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.
aria-hidden="true"is not something that should be used to avoid visible text reading in the wrong place. aria-hidden is more for hiding things which should not be read because they are in the background or are not part of the readable content, like icons, backgrounds, structural stuff, the content underneath popups etc.These are the places we currently use aria-hidden:
https://github.com/search?q=org%3Aadaptlearning%20aria-hidden&type=code
You have to remember that screen readers are not primarily for people who cannot see, but for anyone who needs help reading. Partially sighted people might see various grades of text shapes and outlines, here there will be a clearly important word that is not being read and for no explicit reason.
It would be better to remove the aria-hidden here and have it read twice.
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.
Could you take a screenshot of what this looks like in the boxmenu item and include in the pr description? There's no styling included in the pr, so it would be nice to see what it looks like.
If this were merged and updated it would show on all boxmenu items on all old and new courses. There's no way way to turn this display on/off without removing the _globals required and optional text for the entire course - which probably isn't a good idea as they'll be needed elsewhere I'm sure. Suggest adding a flag, probably at the menu level, to hide/show required/optional on all items?
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.
Just to tac on to this, we do use aria-hidden to hide furniture added by css pseudo elements even though it's permissible - that is an Adapt decision I think. But agree, we shouldn't be using this for text.
This defiantly needs some kind of switch to turn the labels on, as @oliverfoster says, we don't want these appearing randomly in other courses. We just need to be mindful of this when adding in new functionally in general. @oliverfoster suggestion seems like a good one to me. Although it might need some more thought, not sure.