Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ADA-1065-Added aria-labelby attribute to the dialog #896

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ADA-1065-Added aria-labelby attribute to the dialog #896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dc2258
ADA-1065-Added aria-labelby attribute to the dialog
RazvanBrinzoiK05 Jun 20, 2024
5c3a50c
ADA-1065-Removed unnecessary label proprety
RazvanBrinzoiK05 Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/components/cvaa-overlay/cvaa-overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ class CVAAOverlay extends Component<any, any> {
addAccessibleChild={this.props.addAccessibleChild}
onClose={props.onClose}
type="cvaa"
label={props.cvvaDialogText}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove it ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label was passed as a prop just for the aria-label property that was replaced.

>
{this.state.activeWindow === cvaaOverlayState.Main ? (
<MainCaptionsWindow
Expand Down
2 changes: 1 addition & 1 deletion src/components/cvaa-overlay/main-captions_window.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class MainCaptionsWindow extends Component<any, any> {
render(props: any): VNode<any> {
return (
<div className={[style.overlayScreen, style.active].join(' ')}>
<h2 className={style.title}>
<h2 className={style.title} id="captionsDialogTitle">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to avoid using id ?
id should be unique for the page, but we can have multiple players on a single page

Copy link
Author

@RazvanBrinzoiK05 RazvanBrinzoiK05 Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the dialog has a visible title and it must be referenced with aria-labelledby that must point to the title id. If the title would be missing we could've used just an aria-label property that doesn't use id.

Should I find a way to create a dynamic id so that each player has a unique one ?

<Text id={'cvaa.title'} />
</h2>
<div role="group">
Expand Down
4 changes: 2 additions & 2 deletions src/components/overlay/overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ class Overlay extends Component<any, any> {
* @returns {React$Element} - component
* @memberof Overlay
*/
render({type, open, label = 'dialog'}: any): VNode<any> {
render({type, open}: any): VNode<any> {
const overlayClass = [style.overlay];
if (type) {
const classType = style[type + '-overlay'] ? style[type + '-overlay'] : type + '-overlay';
Expand All @@ -140,7 +140,7 @@ class Overlay extends Component<any, any> {
}

return (
<div tabIndex={-1} className={overlayClass.join(' ')} role="dialog" onKeyDown={this.onKeyDown} aria-label={label}>
<div tabIndex={-1} className={overlayClass.join(' ')} role="dialog" onKeyDown={this.onKeyDown} aria-labelledby="captionsDialogTitle">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does tooltip still work if aria-label is removed ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, works the same as before.

<div className={style.overlayContents}>{this.props.children}</div>
{this.renderCloseButton(this.props)}
</div>
Expand Down