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 contrast issue for add icon #850

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Jul 19, 2022

Please check @nickvergessen @nimishavijay :)

I used #3ea857, a darkened hue of var(--color-success) cause that one actually does not have enough contrast. FYI @marcoambrosini @juliushaertl we should probably adjust that?

image

Signed-off-by: Jan C. Borchardt <hey@jancborchardt.net>
@marcoambrosini
Copy link
Member

we should probably adjust that?

Not sure this is the solution, it would make buttons even darker. Our Nextcloud blue fails in the same way, but I don't think we should darken it, we could just avoid using it for text or for tiny important symbols.
Couldn't we avoid the green plus icon and invert it (green background and white icon)?

@jancborchardt
Copy link
Member Author

Not sure this is the solution, it would make buttons even darker. Our Nextcloud blue fails in the same way, but I don't think we should darken it, we could just avoid using it for text or for tiny important symbols.

Not really true: The current color-success is not accessible in any respect, below 3:1 – so we should actually not use it anywhere. Versus the Nextcloud blue which is above 3:1:

color-success Nextcloud blue
image image

@juliushaertl
Copy link
Member

I'd also say darkening that to match the pass on AA would make sense. For passing AAA we could probably darken further just for the high contrast theme?

@jancborchardt
Copy link
Member Author

@juliushaertl yep, sounds good.

I’ll open an issue in the Vue components regarding the color variables.

@juliushaertl
Copy link
Member

server please ;)

@marcoambrosini
Copy link
Member

Oh! I thought that the contrast comparison you posted was the one of the current success color, my bad. I agree then, let's make a server pr and darken it :)

@jancborchardt
Copy link
Member Author

Issue opened at nextcloud/server#33278 :)
Could we merge this then?

@jancborchardt
Copy link
Member Author

And sorry for the confusion @marcoambrosini! :) Should have posted a 2 comparison screenshots instead.

@marcoambrosini
Copy link
Member

btw @jancborchardt this particular icon could still fall in the "Normal text" category, so still not pass the test depending on how it's used. I'm not entirely sure about that

@jancborchardt
Copy link
Member Author

It's a "graphical object" (decorative one at that), and as such it passes. :)

@nickvergessen
Copy link
Member

Contrast on dark mode is also fone?

@nickvergessen nickvergessen merged commit d03d24f into master Jul 20, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/contrast-icon-add branch July 20, 2022 09:13
@jancborchardt
Copy link
Member Author

jancborchardt commented Jul 20, 2022

@nickvergessen for dark mode it needs a different icon, same for the "deleted" icon. There we could also just use the light/white icons?

@nickvergessen
Copy link
Member

WCAG Contrast checker said the color was fine?

Not going to fiddle around and split the color mode based on the theme. If green does not work, we use black/white everywhere

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.

4 participants