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

Add accessibility entry in user menu #34069

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Sep 14, 2022

For #33736

image

Note: if the theming app is disabled the entry will remove itself.

@PVince81 PVince81 added this to the Nextcloud 25 milestone Sep 14, 2022
@PVince81 PVince81 self-assigned this Sep 14, 2022
@PVince81 PVince81 mentioned this pull request Sep 14, 2022
64 tasks
@szaimen

This comment was marked as resolved.

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

I think when in that section also the entry in the nav manager should be highlighted, no?
See
image

@szaimen

This comment was marked as resolved.

Copy link
Member

@nimishavijay nimishavijay 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, a suggestion: we use the A11y icon instead of the theming icon https://fonts.google.com/icons?selected=Material%20Icons%20Outlined%3Aaccessibility_new%3A

@szaimen szaimen mentioned this pull request Sep 14, 2022
@PVince81
Copy link
Member Author

@szaimen I thought of this but don't think it's critical. It would also mean that whenever you manually click on that navigation, the entry on the top right would also need to be highlighted. Might need a hacky solution for this.

@szaimen
Copy link
Contributor

szaimen commented Sep 14, 2022

@szaimen I thought of this but don't think it's critical. It would also mean that whenever you manually click on that navigation, the entry on the top right would also need to be highlighted. Might need a hacky solution for this.

look at https://github.com/nextcloud/server/pull/33756/files#diff-0fc396967e1f9523319e6bbfd0d6877a8a6e5db7a7896697404c19ea896f734dR137-R140. Maybe it helps figuring out a solution

@PVince81
Copy link
Member Author

here we go:
image

@nimishavijay

@szaimen as per screenshot, the active item is now fixed, thanks a lot for the useful pointer

please re-review!

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

lgtm now but acceptance-header failure is related!

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the enh/33736/add-accessibility-in-user-menu branch from 5427aaa to da01494 Compare September 14, 2022 13:06
@PVince81
Copy link
Member Author

solved

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 14, 2022
@szaimen
Copy link
Contributor

szaimen commented Sep 14, 2022

S3 failure unrelated

@szaimen szaimen merged commit 0550e5e into master Sep 14, 2022
@szaimen szaimen deleted the enh/33736/add-accessibility-in-user-menu branch September 14, 2022 14:17
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
@nursoda
Copy link

nursoda commented Sep 19, 2022

I understand the wish to make this submenu easier accessible. However, I think it's inconsistent in terms of "now we have three links to the same side menu" – two main entry points (user, admin) and one sidemenu subentry on top of it. If theming and accessibility were a complete separate side menu, then I'd agree. This way, I don't. But that's not my decision.

BUT: Even the icons used are inconsistent. THAT should be decided upon which one to use consistently:

grafik

@jancborchardt
Copy link
Member

@nursoda yes, understandable – however all of the requirements for having each of the entries quickly accessible are valid. Initially we only had "Settings", then it was cumbersome for admins to get to admin settings quickly, or not clear that this was also there. And "Appearance and accessibility" needs to be reachable quickly to help people in need of assistive technology.

Regarding the icon, yes for sure. @szaimen could you change the icon of "Appearance & accessibility" in the settings to the one we use in the menu (the "human")? Admin theming can keep its icon to distinguish.

@szaimen
Copy link
Contributor

szaimen commented Sep 20, 2022

WIP: #34170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants