[JW-11320]<Teaching Channel 9: No confirmation message is provided it on selecting captions styles 'Reset' button> #3964
Conversation
jdunningjwp
requested review from
Dawolee,
imbcmdth,
jwbrandon,
mamaddox and
zetagame
as code owners
| constructor(_content, _action, _template = itemButtonTemplate) { | ||
| super(_content, _action, _template); | ||
| this.statusEl = document.createElement('span'); | ||
| this.statusEl.id = 'reset-status-message'; |
There was a problem hiding this comment.
jw prefix, use classes rather than ids in case of multiple players per page (id is unique in semantically correct HTML).
| super(_content, _action, _template); | ||
| this.statusEl = document.createElement('span'); | ||
| this.statusEl.id = 'reset-status-message'; | ||
| this.statusEl.style = 'font-size: 0;'; |
There was a problem hiding this comment.
Avoid using CSS in JS whenever possible, as it can't be overwritten by CSS without !important suffixing, which would prevent user customization.
| this.statusEl.style = 'font-size: 0;'; | ||
| this.statusEl.setAttribute('role', 'status'); | ||
| this.el.appendChild(this.statusEl); | ||
| this.el.setAttribute('aria-controls', 'reset-status-message'); |
There was a problem hiding this comment.
This isn't what aria-controls is for, see examples section here: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls
|
Might want to re-write as an aria-live region that gets updated by anything that needs this sort of alert. See here: https://cccaccessibility.org/web-1/web-developer-tutorials/using-aria-live |
Dawolee
force-pushed
the
captions-reset-settings-confirmation
branch
from
8ea017f to
9a3da8d
Compare
|
test this please |
| @@ -35,3 +35,13 @@ export class ButtonMenuItem extends MenuItem { | |||
| this.active = false; | |||
| } | |||
| } | |||
|
|
|||
| export class ResetMenuItem extends MenuItem { | |||
There was a problem hiding this comment.
Should this be in its own file? Something like reset-menu-item?
| @@ -188,9 +188,15 @@ class SettingsMenu extends Menu { | |||
| }); | |||
| captionsSettingsItems.push(resetItem); | |||
| captionsSettingsMenu.setMenuItems(captionsSettingsItems); | |||
| if (isReset) { | |||
| resetItem.statusEl.innerHTML = 'Reset Captions Successful'; | |||
There was a problem hiding this comment.
Might be better to use textContent here and below rather than innerHTML
This PR will...
Add confirmation message "Reset Captions Successful" when the reset button in the captions menu is pressed
Why is this Pull Request needed?
If the screen reader does not notify anything after selecting "Reset" button non-sighted users will not be able to confirm or notice that the information has been reset
Are there any points in the code the reviewer needs to double check?
Nope
Are there any Pull Requests open in other repos which need to be merged with this?
Nope
Addresses Issue(s):
https://jwplayer.atlassian.net/browse/JW8-11320
Checklist