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

Use proper default actions aria label in NcListItem #3714

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

raimund-schluessler
Copy link
Contributor

We currently overwrite the aria-label of the NcActions component used in the NcListItem component with an empty string, which leads to a You need to fill either the text or the ariaLabel props in the button component. triggered by NcListItem by default. This PR sets the default aria-label in NcListItem to the same value as used in NcActions.

@raimund-schluessler raimund-schluessler added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: list-item-icon Related to the list-item-icon component labels Feb 2, 2023
@raimund-schluessler raimund-schluessler added this to the 7.6.0 milestone Feb 2, 2023
@artonge
Copy link
Contributor

artonge commented Feb 2, 2023

I think this is on purpose for accessibility to not have multiple actions with the same aria label

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Feb 2, 2023

I think this is on purpose for accessibility to not have multiple actions with the same aria label

Well, every action has this label unless you overwrite it explicitly. So there are multiple actions with this label on the same page already. Not sure what's the benefit of setting an empty aria-label for some actions, but not all (besides triggering a warning about it 🙈).

@artonge
Copy link
Contributor

artonge commented Feb 2, 2023

Not sure what's the benefit of setting an empty aria-label for some actions, but not all (besides triggering a warning about it see_no_evil).

Then the fix would be to remove the default aria-label on the actions that have a one, no ?
Maybe @Pytal or @JuliaKirschenheuter that are working on accessibility can give us their insights ?

@raimund-schluessler
Copy link
Contributor Author

Then the fix would be to remove the default aria-label on the actions that have a one ?

That would mean making the aria-label prop required, otherwise the NcButton component will complain about every actions component used. That would be a breaking change. And I think devs would then just end up setting the default aria-labels that we just removed in the lib. Input from the accessibility team would indeed be good.

Copy link
Contributor

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Seems fine to me :)

What do you think @JuliaKirschenheuter?

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler changed the title Don't overwrite actions aria label in NcListItem Use proper default actions aria label in NcListItem Feb 3, 2023
@raimund-schluessler
Copy link
Contributor Author

/l10n-update

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@raimund-schluessler raimund-schluessler merged commit 1098296 into master Feb 3, 2023
@raimund-schluessler raimund-schluessler deleted the fix/noid/list-item-aria branch February 3, 2023 15:04
@Pytal Pytal mentioned this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: actions Related to the actions components feature: list-item-icon Related to the list-item-icon component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants