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

feat: added custom condition to the aria-label user button #6346

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Wagner3UB
Copy link
Contributor

Added a condition to set the aria-label to differentiate the "regular users" from the admin users.

The condition is the same one used inside the 'PersonalTools' component, which opens after the user button is clicked.

Issue with discussion:
#5119

@Wagner3UB Wagner3UB added the 99 tag: UX Accessibility Accessibility issues label Sep 27, 2024
@Wagner3UB Wagner3UB self-assigned this Sep 27, 2024
Copy link

netlify bot commented Sep 27, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 3189875
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/670e31c6e916760008cc12e3

Copy link
Member

@JeffersonBledsoe JeffersonBledsoe left a comment

Choose a reason for hiding this comment

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

Left a couple of comments. One of them will need to be resolved in a follow up PR, but I'm happy with this once the other minor things are covered!

packages/volto/src/components/manage/Toolbar/Toolbar.jsx Outdated Show resolved Hide resolved
packages/volto/news/6346.feature Outdated Show resolved Hide resolved
packages/volto/src/components/manage/Toolbar/Toolbar.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Per a11y team meeting

packages/volto/news/6346.feature Outdated Show resolved Hide resolved
packages/volto/news/6346.feature Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Should we remove the messages for Personal Tools, since this PR seems to make it unnecessary?

packages/volto/src/components/manage/Toolbar/Toolbar.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

This is nice! Just a minor tweak to the news, and LGTM.

packages/volto/news/6346.breaking Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Pending passing CI, of course.

Copy link
Member

@JeffersonBledsoe JeffersonBledsoe left a comment

Choose a reason for hiding this comment

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

Looks good!
I'm going to re-run the readme link check, as it's a little flakey and failed on https://www.ueu.eus/ , even though the site is able to be accessed

@JeffersonBledsoe
Copy link
Member

@stevepiercy Any idea what's up with the failing ReadTheDocs CI step?

@stevepiercy
Copy link
Collaborator

@sneridagh I'd ignore it as it is for the deleted RTD project, and was probably due to our fiddling around with it, based on the timing.

@JeffersonBledsoe
Copy link
Member

@Wagner3UB Can you change the label to 24 status: ready if there's nothing left to do in this PR?

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.

5 participants