Update: provide alternative title text override for menu button ARIA label (fixes #214) #215
Conversation
...altTitle is already a supported contentObject property used to provide an alternative on screen title for PLP adaptlearning/adapt-contrib-pageLevelProgress#208
swashbuck
left a comment
swashbuck
left a comment
There was a problem hiding this comment.
👍
@kirsty-hames Looks good! Will need an update to the Box Menu migration scripts. I think that would be a separate ticket since we don't know what the exact release version would be?
Hi @joe-allen-89, do we have a defined practice for any PRs that require migration scripts? |
That's a terrifying (and probably true) comment. |
|
@kirsty-hames I'm currently looking at github actions to see if there's a way we can automatically raise an issue on the repo if the schema has been updated. Agree with what Brad said, as we don't really know the release version it would be better to have it as a seperate issue/PR (I think) |
|
This sounds problematic. Example: The schema changes are released on 1.0.0. The automation changes are added to 1.0.1 (or to master without release). When someone installs 1.0.0, the schema will change, but the automation tasks won't exist in that version. |
The other option is validation to prevent release if the schema has been updated but migration script hasn't, but will absolutely every schema change require a migration script? Possibly not. And still leaves us with having to come in and manually update the migration script as we release to ensure the versions are correct though. Hmm, bit more thought needed on that. |
That's a good point. I'm assuming instances where a new property is added that is either optional or a default value is provided, wouldn't require a migration script? If that's the case, do we need to add a migration script here as For PRs that do require a migration script, could this be handled within the same PR but we have two review phases? For example, |
| return boxMenus.length; | ||
| }); | ||
|
|
||
| mutateContent('Box menu - add titleAriaLabel to contentObjects', async (content) => { |
There was a problem hiding this comment.
I don't think this is working. Going to take a look.
There was a problem hiding this comment.
@kirsty-hames I think the issue is that the properties and migration scripts assume the new titleAriaLabel property to be part of the _boxMenu object on a content object. However, in the example.json and README.md, the property sits alongside displayTitle and body at the root of the content object.
Which way should this work? For instance, are the schemas correct or are the examples correct?
There was a problem hiding this comment.
Good spot @swashbuck! I had intended for property to sit alongside displayTitle and body however seeing as this is an optional property and not a common use case, perhaps it would be best part of the _boxMenu object not to clutter up content object?
There was a problem hiding this comment.
@kirsty-hames Sure, having it part of the _boxMenu object makes sense to me. Can you make that change, and I'll test out the migration scripts again?
Fixes #214
Update
titleAriaLabeloverride provided to be read out by screen readers, instead oftitlefor the menu item button ARIA label. To be used whentitlecontains punctuation or html syntax that shouldn't be read. IftitleAriaLabelisn't set, thentitleis used to label the menu item button as default.Note, as per the issue comment, I'd initially suggested naming the override property
altTitlefor consistency with the alternative title created for question feedback. However contentObjectaltTitleis already supported to provide an on screen alternative title for PLP. As the two use cases would require separate titles, I decided to create an separate property foraria-labelonly.Example use case
title: Being ‘on-spec’ (on screen text)titleAriaLabel: Being on spec (aria-labeltext with no punctuation, spelt as should be read)