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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 14 commits into 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
@netlify Netlify
Copy link

netlify bot commented Sep 27, 2024 •
edited
Loading

✅ Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 270d7c3
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/672089c170a3bf0008acc2ff

JeffersonBledsoe
JeffersonBledsoe requested changes Sep 27, 2024
View reviewed changes
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 Show resolved Hide resolved
packages/volto/src/components/manage/Toolbar/Toolbar.jsx Outdated Show resolved Hide resolved
@JeffersonBledsoe JeffersonBledsoe mentioned this pull request Sep 30, 2024
Closed
stevepiercy
stevepiercy requested changes Oct 1, 2024
View reviewed changes
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
stevepiercy
stevepiercy reviewed Oct 1, 2024
View reviewed changes
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
@stevepiercy stevepiercy mentioned this pull request Oct 3, 2024
Open
stevepiercy
stevepiercy requested changes Oct 14, 2024
View reviewed changes
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
stevepiercy
stevepiercy approved these changes Oct 14, 2024
View reviewed changes
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.

JeffersonBledsoe
JeffersonBledsoe approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@JeffersonBledsoe JeffersonBledsoe left a comment •
edited
Loading

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?

@ichim-david
Copy link
Member

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

@Wagner3UB I agree with Steve if we no longer use Personal Tools I would remove it.
I would also like to get translations for these 2 new strings at least for the languages that had Personal tools translated.
I don't think it's fine for us to replace their translated strings with untranslated english strings.
We can always also CC other translators to double-check if what Google Translate or other services suggested we use is accurate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevepiercy stevepiercy stevepiercy approved these changes

@JeffersonBledsoe JeffersonBledsoe JeffersonBledsoe approved these changes

@pnicolli pnicolli Awaiting requested review from pnicolli

@SaraBianchi SaraBianchi Awaiting requested review from SaraBianchi

@ichim-david ichim-david Awaiting requested review from ichim-david

Assignees

@Wagner3UB Wagner3UB

Labels
22 status: in-progress 99 tag: UX Accessibility Accessibility issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Wagner3UB @JeffersonBledsoe @stevepiercy @ichim-david @pnicolli