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

test(cv-link): add tests for a11y #1539

Merged

Conversation

halivert
Copy link
Contributor

@halivert halivert commented Oct 4, 2023

Contributes to #1538

What did you do?

Add a11y test for CvLink

Why did you do it?

Adding 3 test cases, for simple link, one in disabled state and the other one with an icon

How have you tested it?

Using testing library, the icon one is failing because needs a label, but I'm not sure how to proceed

Notes

I have an idea of naming convention for accessibility tests, maybe we can use the abbreviation, [Component].a11y.spec.js?

Thank you in advance

@halivert halivert marked this pull request as draft October 4, 2023 14:23
@davidnixon
Copy link
Collaborator

I updated the guidance a bit to add a link to the React storybook. There might be DOM difference for accessibility there that might be helpful in resolving any test issues.

The accessability rules seems to have changed since Carbon 10 was relesed but still it might be helpful to reference the React Carbon 10 Storybook to see if there are difference in the DOM for accessability.

@davidnixon
Copy link
Collaborator

I think the test cases can be resolved by adding alt="" to the component

<CvSvg v-if="icon" :class="`${carbonPrefix}--link__icon`" :svg="icon" />

@halivert
Copy link
Contributor Author

Thanks @davidnixon, I wasn't sure if I should update actual component, and I had a lot of work lately, but thanks for your advice 👍🏽
Done in 0d55fcc

@halivert halivert marked this pull request as ready for review October 29, 2023 23:22
v-if="icon"
:class="`${carbonPrefix}--link__icon`"
:svg="icon"
alt=""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty alt because it's "an advisory icon of minimal importance"

Copy link
Collaborator

@davidnixon davidnixon left a comment

Choose a reason for hiding this comment

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

lgtm

@kodiakhq kodiakhq bot merged commit 688af4e into carbon-design-system:main Nov 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants