New: Required and optional labels added (Fixes #191) #193
Conversation
| @@ -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.
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.
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.
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.
| @@ -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}}{{/if}}"{{#if _isLocked}} aria-disabled="true"{{/if}} role="link"> | |||
| <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
| @@ -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.
| <div class='menu-item__priority boxmenu-item__priority' aria-hidden="true"> | |
| <div class='menu-item__priority boxmenu-item__priority'> |
oliverfoster
changed the title
|
Yes please to a mock-up. Personally, I think the required and optional text should sit above all the other text. It seems more logical there as you need to know first whether the text is 'required' reading. It would be good if it sits outside the flow of the other content (I think) as that'll give more options for placement of that text (so at the top of the item 'box' for example. I'd also like to see some styling perhaps, as this feels like a separate thing from the rest of the content of that 'box' item. |
|
I'll leave the mock-up for the boxmenu item to @zubair but I've added a simple mock-up of a suggested use case for discussion
|
Fixes #191
New