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

3477 tweak colors for contrast #3921

Merged
merged 4 commits into from
May 10, 2024

Conversation

Lakoja
Copy link
Collaborator

@Lakoja Lakoja commented Aug 4, 2023

Fixes: #3477

This tweaks two colors:

  • for the dark theme a new slightly darker (timeline) background is used.
  • for the light theme a darker shade of blue is used as highlight color (colorPrimary and colorSecondary)

Additional change:

  • Use green and red more consistent: use existing colors for the diff display; move the new light variants to colors.xml like all colors

@nikclayton
Copy link
Contributor

(copied out of the initial comment, and adjusted to sit side by side)

Visual comparisson (first before, then after):

Copy link
Contributor

@nikclayton nikclayton left a comment

Choose a reason for hiding this comment

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

This changes more than the two colours that are in the PR description. Is that intended?

@nikclayton
Copy link
Contributor

I don't think the colour of the FAB has to change, based on https://webaim.org/resources/contrastchecker/.

If I put in #ffffff as the foreground colour (ie., the pencil icon) and #2B90D9 as the background colour it reports that as a pass for "Graphical Objects and User Interface Components"

@Lakoja
Copy link
Collaborator Author

Lakoja commented Aug 7, 2023

I don't think the colour of the FAB has to change, based on https://webaim.org/resources/contrastchecker/.

I guess that is what this PR is also partly about: Be consistent in color usage.

So if the FAB and text highlights are presented in colorSecondary they can and should keep it that way when that is changed.

Same goes for the green: If there is one, use it. (And change it here to improve the contrast.)

@Lakoja
Copy link
Collaborator Author

Lakoja commented Aug 7, 2023

This changes more than the two colours that are in the PR description. Is that intended?

Add a part for red and green to the description.

@Lakoja Lakoja force-pushed the 3477-tweak-colors-for-contrast branch from effacfc to 6d43679 Compare August 7, 2023 08:02
@Lakoja Lakoja force-pushed the 3477-tweak-colors-for-contrast branch 2 times, most recently from d14fce5 to aabf59b Compare August 17, 2023 19:33
@nikclayton
Copy link
Contributor

nikclayton commented Aug 19, 2023

I think this is more complex than it needs to be for what it is trying to accomplish.

The original issue (#3477) is just about the link colour. This PR changes more than that, and the description is incorrect w.r.t what it currently changes.

With this PR:

Light mode

  • Links are darker (#2b90d9 -> #217aba)
  • "Widget blue" (FAB, tab icons, slider thumb, etc) is darker (#2b90d9 -> #217aba) (undocumented change)
  • Diff green is the same (#ccffd8)
  • Diff red is the same (#ffc0c0)

Dark mode

  • No difference to background (#282c37), contrary to PR description
  • FAB is different colour (#2b8fd7 -> #2b90d9) (undocumented change)
  • Tab icons are the same colour (#2b90d9)
  • Slider thumb is the same colour (#2b90d9)
  • Diff green is the same (#00731b)
  • Diff "red" is now purple (#df0000 -> #df1553)

The suggested new link colours from #3477 (comment) are:

  • #217aba -- link colour in light mode
  • #3c9add -- link colour in dark mode

That can be achieved by:

  1. Edit values/theme_colors, add an entry <color name="colorAccent">#217aba</color>
  2. Edit values-night/theme_colors, add an entry <color name="colorAccent">#3c9add</color>
  3. Edit values/styles.xml, add an entry <item name="colorAccent">@color/colorAccent</item> in the TuskyBaseTheme style
  • In both modes that will adjust the link colour as intended.
  • In dark mode the colours of the FAB, tab icons, and widgets (e.g., the slider thumb on the font size preference) are unchanged, also as intended
  • In light mode the colours of the tab icons and widgets are unaffected. The colour of the FAB goes fractionally darker, #2b90d9 -> #2b8fd8

@Lakoja
Copy link
Collaborator Author

Lakoja commented Aug 19, 2023

I think this is more complex than it needs to be for what it is trying to accomplish.

I guess it's not quite as complicated as listed:

  • It should unify color handling a bit - there is for example no need for very special reds and greens.
  • It improves contrast between color secondary and background where needed - and that leads to some consequences (e. g. fab color).

That is sensible and not too big a change for a PR.

@nikclayton
Copy link
Contributor

OK, please split this in two:

1 PR -- just the accessibility changes for changing the link colours. Those are non contentious, solve a problem that users have identified and filled an issue for, and are straightforward to approve. I think the colorAccent changes I describe above are the smallest, simplest, way to do this, but if you have an approach that is even simpler then I'm happy to look at that too. Please do not change the background colour, that's a much broader change than something that just affects the link colours.

Discussion -- if you want to propose additional colour changes, or how the colours / styles / themes are currently organised (and I do see options for how they can be reorganised / updated) please do that in either a new issue or a discussion in the Matrix channels.

Thanks.

@Lakoja Lakoja force-pushed the 3477-tweak-colors-for-contrast branch from aabf59b to c4573ca Compare April 26, 2024 13:08
@connyduck
Copy link
Collaborator

The tusky_blue is still used in some places with the light theme, e.g. The "Lock account" checkbox when editing a profile, in the elephant friend drawables, and in the spinners of SwipeToRefreshLayouts.

@Lakoja
Copy link
Collaborator Author

Lakoja commented May 9, 2024

The tusky_blue is still used in some places with the light theme, e.g. The "Lock account" checkbox when editing a profile, in the elephant friend drawables, and in the spinners of SwipeToRefreshLayouts.

As far as I remember from a year ago I (only) wanted to fix the problem at hand: low contrast for text and or links.

But I guess we could do the mentioned problems in some subsequent merges.
I would feel confident to unify the SwipeToRefreshLayouts to a base layout with the proper respective color - but not in this PR.

For the others I might need to research a bit more.

I wonder however if I should include the red character warning number here - as it is really eye-watering with the dark theme...

grafik

@connyduck
Copy link
Collaborator

Ok I'm gonna merge this for now. Please send more PRs, and yes that red text is very important.

@connyduck connyduck merged commit b60e111 into tuskyapp:develop May 10, 2024
3 checks passed
connyduck added a commit that referenced this pull request Jun 19, 2024
Follow up to #3921

- no more hardcoded `tusky_blue`, instead the `colorPrimary` attribute
is used. This will help us when adding more themes, e.g a dynamic color
one.
- The `colorPrimary` of the dark theme is now lighter for more contrast
and subsequently the `colorOnPrimary` is now dark grey instead of white.
- `tusky_red_lighter` is now a bit more red than before
- Tweaked color usage in a few places for better contrast

I think this looks a bit unfamiliar but overall better and the higher
contrast makes things noticeably easier to read.

<img
src="https://github.com/tuskyapp/Tusky/assets/10157047/4cbb92d8-b772-4e94-bc15-c4baf0e5473f"
width="260"/>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links show too little contrast in dark mode
3 participants