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

color-text-maxcontrast needs to use darker value in new left navigation #3198

Closed
jancborchardt opened this issue Sep 8, 2022 · 6 comments · Fixed by #3307
Closed

color-text-maxcontrast needs to use darker value in new left navigation #3198

jancborchardt opened this issue Sep 8, 2022 · 6 comments · Fixed by #3307
Assignees
Labels
1. to develop Accepted and waiting to be taken care of accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: app-navigation Related to the app-navigation component high High priority ui-refresh-feedback

Comments

@jancborchardt
Copy link
Contributor

jancborchardt commented Sep 8, 2022

Since the new left navigation is slightly transparent, we need to use a slightly darker --color-text-maxcontrast value when it’s used in there.

To be clear: Only for the usage in there, we should not increase the value of the variable overall, as then it will reduce the contrast/difference to --color-main-text.

Of course there are different backgrounds, but let’s start out by making sure it works for the default background.

With current #767676 (maxcontrast) for subline With better #646464
image image
image image

Also, .line-two__subtitle uses the outdated variable var(--color-text-lighter) – we only use --color-main-text and --color-text-maxcontrast, as per the design guidelines: https://docs.nextcloud.com/server/latest/developer_manual/design/foundations.html#text-color

@jancborchardt jancborchardt added 1. to develop Accepted and waiting to be taken care of feature: app-navigation Related to the app-navigation component accessibility Making sure we design for the widest range of people possible, including those who have disabilities ui-refresh-feedback labels Sep 8, 2022
@jancborchardt
Copy link
Contributor Author

@juliushaertl @CarlSchwan we should ideally fix this for the release as it relates to the accessibility of a very prominent element. What do you think of the proposal?

@jancborchardt
Copy link
Contributor Author

cc @AndyScherzinger

@jancborchardt jancborchardt added high High priority bug Something isn't working labels Sep 20, 2022
@AndyScherzinger
Copy link
Contributor

Sounds good to me 👍
Also the change is very subtle - thus not impacting screenshots

@Pytal
Copy link
Contributor

Pytal commented Sep 27, 2022

This uses server-side calculated color variables so splitting up calculations with new client-side logic wouldn't be an option, seems that adding a new variable would be the way to go then @skjnldsv?

Would mean replacing --color-text-lighter in NcListItem with the new variable

@Pytal Pytal self-assigned this Sep 27, 2022
@jancborchardt
Copy link
Contributor Author

jancborchardt commented Sep 27, 2022

@Pytal note that --color-text-lighter and --color-text-light are actually not recommended to use. We use:

  • --color-main-text
  • --color-text-maxcontrast

And while we could introduce a new --color-text-maxcontrast-more or something like that, it would even be better if we just say that on certain backgrounds (like the one in the navigation), the maxcontrast is just upped a little. That way developers don’t have to specifically set it, and also sometimes the background of the navigation might not be transparent, like on mobile.

@Pytal
Copy link
Contributor

Pytal commented Sep 28, 2022

We can only increase the contrast by darkening the variable server-side with $this->util->darken($colorTextMaxcontrast, 7) (#767676 -> #646464) as CSS does not allow you to darken a color variable at runtime and we have to scope it to maxcontrast text within AppNavigation with a light theme applied

Fix in #3307 and nextcloud/server#34299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: app-navigation Related to the app-navigation component high High priority ui-refresh-feedback
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants