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

users: Add screen reader accessible column #21546

Merged

Conversation

Venefilyn
Copy link
Contributor

Current users page lists the users in a table together with a column for actions. However, the table header doesn't include the last actions column and instead shows the table header line ending preemptively.

As we likely don't want a visibly named column for the actions this will instead visually just extend the line so it is consistent while also adding a screen reader friendly text to indicate what the column is portraying.

This does mean we need to include
@patternfly/patternfly/patternfly-addons.css in the PatternFly CSS import.


This line seems only to occur on Chrome and not Firefox, but definitely a bug
image

PR applied:
image

@martinpitt
Copy link
Member

Fun! Our current pixel test reference is correct, the separator lines properly go to the end. Triggering the f-41 tests to see if it makes any difference.

I cannot reproduce the bug with Chromium 132.0.6834 (current F41 version). Nevertheless, this looks harmless/ok, so I'm fine with applying it. However, I have a question:

We don't use the pf-v5-u-screen-reader anywhere in our projects so far, so that's new to me. Documentation.

There are only two other similar usages:

pkg/packagekit/updates.jsx: {_("Danger alert:")}
pkg/static/login.html:

The latter seems to be an alias according to the documentation. @Venefilyn do you know the difference between the variant with and without -u? And I think the first one is just entirely outdated and ought to be removed?

Thanks!

@martinpitt
Copy link
Member

@Venefilyn Ugh, this breaks mobile screen size quite badly -- it seems to allocate a lot of extra space for the new column and then keep it empty.

This failure is also curious -- at first sight it's totally unrelated, but then again the font change doesn't feel like a flake. I've never seen that kind of flake before.

So this is a typical Yak shave 😢

@Venefilyn
Copy link
Contributor Author

@Venefilyn Ugh, this breaks mobile screen size quite badly -- it seems to allocate a lot of extra space for the new column and then keep it empty.

I'll take to make sure mobile view works as expected, thanks!

This failure is also curious -- at first sight it's totally unrelated, but then again the font change doesn't feel like a flake. I've never seen that kind of flake before.

It could be related either way since this PR does include a CSS import. It shouldn't do anything given they are helper CSS stuff but the helper classes might be used here and there without them doing anything atm

@Venefilyn Venefilyn force-pushed the users/add-screen-reader-column branch from 122bcbf to 708ae6d Compare January 27, 2025 12:30
@Venefilyn
Copy link
Contributor Author

@martinpitt the failing pixel test look fine now to me if I read them correctly

@martinpitt
Copy link
Member

@Venefilyn Agreed, this looks reasonable. I need to run, please ask @jelly or someone else in the channel to push the pixel test. Otherwise I'll do it this evening. 👍 thanks!

Current users page lists the users in a table together with a column
for actions. However, the table header doesn't include the last actions
column and instead shows the table header line ending preemptively.

As we likely don't want a visibly named column for the actions this will
instead visually just extend the line so it is consistent while also
adding a screen reader friendly text to indicate what the column is
portraying.
@martinpitt martinpitt force-pushed the users/add-screen-reader-column branch from 708ae6d to 5606783 Compare January 27, 2025 16:01
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

I pushed the pixel test (you need repo permissions for that -- it will come soon enough 😁 ). Thanks!

@martinpitt martinpitt merged commit 326561b into cockpit-project:main Jan 27, 2025
85 checks passed
@Venefilyn Venefilyn deleted the users/add-screen-reader-column branch January 27, 2025 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants