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

fix(ui): a11y test #1161

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

Conversation

bar-tay
Copy link
Contributor

@bar-tay bar-tay commented Sep 26, 2024

closes #778

@bar-tay bar-tay added the 🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers label Sep 26, 2024
Copy link
Contributor

@thrbnhrtmnn thrbnhrtmnn left a comment

Choose a reason for hiding this comment

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

Hey @bar-tay ,
i still find a11y errors in the toggle switch when looking at the component. It also seems like we have more errors now than the one that is stated in the ticket. We should fix all errors with this PR:

  • Violation: "Elements in the focus order should have an appropriate role", happens in all states
  • Incomplete: "Form field must not have multiple label elements", happens when hasStateLabel=true and label=true
  • Violation: "Form elements must have labels", happens if hasStateLabel=false and label=false; here I am not sure if this is okay as this variant of the component would require a text somewhere next to it which would need to function as the label

Furthermore I could not check the dark mode anymore because it broke with the PR. On develop the dark mode is still working.

# Conflicts:
#	packages/ui-library/src/components/toggle-switch/index.ts
@bar-tay
Copy link
Contributor Author

bar-tay commented Oct 16, 2024

The approach I attempted to implement did not work as expected. It would either break the component or generate new accessibility errors. The "Violation" error in the default and other states, which was apparently already present in the develop branch, persisted even when I applied the appropriate role to the component.
It seems to me that this component might need some restructuring to resolve all the accessibility errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🦹 needs:reviewers (code) The issue or pull request needs additional code reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToggleSwitch - fix accessibility violations
3 participants