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

chore(test): add aria labels for rows #6826

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Conversation

cbr7
Copy link
Contributor

@cbr7 cbr7 commented Apr 18, 2024

What does this PR do?

Add aria-labels for images and container page rows

What issues does this PR fix or reference?

#5639

How to test this PR?

Check image and container rows to have aria-label property with the corresponding values.

Signed-off-by: Vladimir Lazar <vlazar@redhat.com>
@cbr7 cbr7 linked an issue Apr 18, 2024 that may be closed by this pull request
@cbr7 cbr7 added area/tests 🚦 kind/enhancement ✨ Issue for requesting an improvement area/ui labels Apr 18, 2024
cbr7 and others added 2 commits April 18, 2024 11:40
@cbr7 cbr7 marked this pull request as ready for review April 18, 2024 09:57
@cbr7 cbr7 requested review from benoitf and a team as code owners April 18, 2024 09:57
@cbr7 cbr7 requested review from dgolovin, jeffmaury and a team and removed request for a team April 18, 2024 09:57
Copy link
Contributor

@ScrewTSW ScrewTSW left a comment

Choose a reason for hiding this comment

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

LGTM

@cbr7 cbr7 merged commit ef62290 into containers:main Apr 18, 2024
8 checks passed
@cbr7 cbr7 deleted the rows_aria_labels branch April 18, 2024 11:40
@podman-desktop-bot podman-desktop-bot added this to the 1.10.0 milestone Apr 18, 2024
Copy link
Contributor

@odockal odockal left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like to have someone from developer to take a look at tests.

@deboer-tim
Copy link
Collaborator

FWIW the reason I didn't do this before is that name isn't unique, and there's no guarantee that any new item we decide to put in a table will have a name (although most of them appear to have one now). If the tests don't have situations where there are (e.g.) multiple images with the same name, then it'll be fine for now.

@odockal
Copy link
Contributor

odockal commented Apr 18, 2024

No problem, I just could see possible ambiguity during testing if there are same names used in labels, but that could be workarounded with adding another role/name check. The unit test relying on multiple tooltips seemed awkward to me, that is all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tests 🚦 area/ui kind/enhancement ✨ Issue for requesting an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ARIA functionality to image/containers/pods table rows
5 participants