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.

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

Accessibility in accordion panel #537

Merged
merged 5 commits into from
Feb 14, 2023

Conversation

brichet
Copy link
Contributor

@brichet brichet commented Feb 13, 2023

This PR fixes an accessibility issue about accordion panel:

  • removes the role tab to the title, which is not in a tablist element.
  • adds an ID to the widgets if necessary, to correctly link the aria-controls attribute of the corresponding title.

@brichet brichet changed the title Adds IDs to the widgets of accordion panel Accessibility in accordion panel Feb 13, 2023
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @brichet

I guess you use the Renderer to reuse the uuid. But could you please prefer not changing any public API and add the id if it does not exist in AccordionLayout.insertWidget - inherited from https://github.com/jupyterlab/lumino/blob/main/packages/widgets/src/panellayout.ts#L87

@fcollonval fcollonval added bug Something isn't working accessibility Addresses accessibility needs labels Feb 13, 2023
@brichet
Copy link
Contributor Author

brichet commented Feb 14, 2023

I guess you use the Renderer to reuse the uuid. But could you please prefer not changing any public API and add the id if it does not exist in AccordionLayout.insertWidget - inherited from https://github.com/jupyterlab/lumino/blob/main/packages/widgets/src/panellayout.ts#L87

Thanks for review @fcollonval

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @brichet for the changes.

Could you add assertions to the tests and run yarn run api to update the API files (the associated CI job is failing).

let w2 = new Widget();

// Expects a widget ID to be created.
expect(!w1.id);
Copy link
Member

Choose a reason for hiding this comment

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

I think you cannot just call expect, you need to add an assertion like

expect(w1.id).to.be.undefined;

For test equality, you should use expect(a).to.equal('value');

You can look at the chai library to see what is available.

Could you fix the test at line 123 that probably misguided you as it was missing an assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make sure the tests pass 😄
Thanks for noticing.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @brichet

@fcollonval fcollonval merged commit d08b6f7 into jupyterlab:main Feb 14, 2023
@brichet brichet deleted the accordion-accessibility branch February 14, 2023 20:21
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Addresses accessibility needs bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants