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

fix(a11y): contrast for active menubar buttons #5098

Merged
merged 4 commits into from
Dec 28, 2023

Conversation

max-nextcloud
Copy link
Collaborator

📝 Summary

🖼️ Screenshots

🏚️ Before 🏡 After
grafik grafik

Dropdown menus (headings and callouts) are still low contrast:

grafik

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • we do not test UI look yet.
  • documentation is not required.

@juliusknorr
Copy link
Member

Thinking more about the dropdown, maybe it is already fine like it is as the primary trigger button already indicates the state?

@max-nextcloud
Copy link
Collaborator Author

Thinking more about the dropdown, maybe it is already fine like it is as the primary trigger button already indicates the state?

I'd be happy to merge this as is as a start and then discuss how to continue.

@juliusknorr
Copy link
Member

  • On light mode the color does not seem to fit.
  • And I'm wondering if we shall give those a gap of 2px to make it look a bit nicer when two besides each other are active?
Screenshot 2023-12-07 at 10 44 45

@juliusknorr juliusknorr added this to the Nextcloud 29 milestone Dec 7, 2023
@szaimen
Copy link
Contributor

szaimen commented Dec 8, 2023

  • On light mode the color does not seem to fit.

Changing

fill: var(--color-main-text);
to fill: var(--color-primary-element-text) should most likely fix it

@max-nextcloud
Copy link
Collaborator Author

I fixed the light colors by removing the customized styling and relying on the defaults:

dark light
grafik grafik

@max-nextcloud
Copy link
Collaborator Author

And I'm wondering if we shall give those a gap of 2px to make it look a bit nicer when two besides each other are active?

grafik

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 from the screenshots but didnt test.

Maybe we should loop in Frank because it is kind of a drastic design change again?

@max-nextcloud
Copy link
Collaborator Author

Maybe we should loop in Frank because it is kind of a drastic design change again?

Let's check with @jancborchardt then.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

We do this the same in Office already, so yep while it is very present, accessibility- and consistency-wise this is a good solution. :)

@juliusknorr
Copy link
Member

Cypress failures seem related

@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Dec 18, 2023

Cypress failures seem related

I think the failure on runner 3 actually is related.
Mobile actions -- formatting modal help (failed)

Due to the larger spacing between buttons the more button is overlapped.

Maybe leave out the spacing when on mobile?

@max-nextcloud
Copy link
Collaborator Author

Removed the padding on mobile views (<768 according to texts definition).

Bildschirmaufzeichnung.vom.21.12.2023.08.11.03.webm

@max-nextcloud
Copy link
Collaborator Author

/backport to stable28

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter 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, thanks a lot!

@max-nextcloud
Copy link
Collaborator Author

Cypress tests are still failing and the failure seems to be related.

Dropdown menus (headings and callouts) are still low contrast.

Signed-off-by: Max <[email protected]>
* Use default (white) text in active buttons in light mode.
* Add a 2px margin between menubar buttons to have a bit of spacing
  when two buttons are active next to each other.

Signed-off-by: Max <[email protected]>
Also fix the computation of the number of slots available

Signed-off-by: Max <[email protected]>
@max-nextcloud max-nextcloud merged commit 38b9b7e into main Dec 28, 2023
37 checks passed
@max-nextcloud max-nextcloud deleted the fix/5065-menubar-a11y branch December 28, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants