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(tags): tag text color contrast #3653

Merged
merged 47 commits into from
Jan 17, 2023
Merged

Conversation

Ornanovitch
Copy link
Contributor

@Ornanovitch Ornanovitch commented Sep 25, 2022

Fixes #3633

Depends on #3652

Changes proposed in this pull request:

Use the new isDark utility to add new classes (e.g. tag-dark or tag-light) for better contrasts between tag's bg and text.

Reviewers should focus on:

My code style?

Screenshot

before after
discussion list Screenshot 2022-09-25 at 18-28-23 flarobe Screenshot 2022-09-25 at 18-27-31 flarobe
admin edit modal Screenshot 2022-09-26 at 10-10-57 flarobe Screenshot 2022-09-26 at 10-11-28 flarobe
tags page Screenshot 2022-09-26 at 10-07-41 Tags - flarobe Screenshot 2022-10-18 at 20-07-46 Tags - flarobe
discussion list hero Screenshot 2022-09-29 at 15-47-59 General - flarobe Screenshot 2022-09-29 at 15-59-34 General - flarobe
discussion hero Screenshot 2022-09-29 at 16-24-49 hello the dark world - flarobe Screenshot 2022-09-29 at 16-25-06 hello the dark world - flarobe
"start a discussion" btn Screenshot 2022-09-29 at 16-27-45 General - flarobe Screenshot 2022-09-29 at 16-39-41 General - flarobe

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@Ornanovitch
Copy link
Contributor Author

There is maybe a last element to work on, but it's less trivial, I need to think a bit more about it:

Screenshot 2022-10-15 at 14-23-02 zfeez - flarobe

Other than that, I think my proposal is ready :)

@Ornanovitch Ornanovitch marked this pull request as ready for review October 15, 2022 12:26
@Ornanovitch

This comment was marked as outdated.

@Ornanovitch

This comment was marked as resolved.

@SychO9
Copy link
Member

SychO9 commented Nov 26, 2022

Yea, this is where CSS variables help with specificity issues. Let me push a commit later for this 👍🏼

Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Pushed a commit to make it more flexible to change color based on YIQ using CSS Variables without having to define modifier classes for every component.

I also change the #111 & #ddd colors to #000 & #fff. They easier to the eyes, the other colors looked really odd with some backgrounds.

Another thing I noticed while testing locally 🤔. Too many other colors are changing that I don't think we want them changed. That's due to the threshold of 128 which insures high contrast results. That's good for a11y but I believe we want to introduce a high contrast mode in the future for this specific case rather than change all default coloring.

Can we please increase the threshold to 138?

@Ornanovitch
Copy link
Contributor Author

Wow, what an improvement! Thank you 🚀 🚀 🚀 I need to learn about those css variables tricks.

I also change the #111 & #ddd colors to #000 & #fff. They easier to the eyes, the other colors looked really odd with some backgrounds.

That's fine IMO

Too many other colors are changing that I don't think we want them changed. That's due to the threshold of 128 which insures high contrast results. That's good for a11y but I believe we want to introduce a high contrast mode in the future for this specific case rather than change all default coloring.

Can we please increase the threshold to 138?

I don't have strong opinion on this, but I'd like to test a little. Do you have examples of some unexpected color changes?

@SychO9
Copy link
Member

SychO9 commented Nov 26, 2022

I'd like to test a little. Do you have examples of some unexpected color changes?

I use tag colors from discuss, the white text color on support tag by default is fine, but isDark with the current threshold of 128 leads in dark text color. (same with 90% tags).

Like I mentioned before, it's actually better for a11y for people who need really high contrast, but the intention is to fix that in the future through a high contrast mode anyway.

@SychO9 SychO9 changed the title fix: tag contrast fix(tags): tag text color contrast Nov 26, 2022
@Ornanovitch
Copy link
Contributor Author

Tested, it seems perfect to me :)

1
3

@Ornanovitch Ornanovitch requested review from SychO9 and removed request for davwheat November 26, 2022 23:04
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

The last commit I pushed preserves design colo consistency in both light & dark modes.
This looks ready to be merged now.

@iamdarkle
Copy link
Member

This is really cool, congrats on the long work on this!!

I'm glad I'll soon be able to get rid of my CSS mess that was trying to match the color with each of my tags.

@SychO9 SychO9 added the type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) label Dec 4, 2022
@SychO9 SychO9 added this to the 1.7 milestone Dec 4, 2022
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Very cool!

@SychO9 SychO9 merged commit 4d29226 into flarum:main Jan 17, 2023
@SychO9 SychO9 added the javascript Pull requests that update Javascript code label Feb 11, 2023
@luceos luceos mentioned this pull request Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tags] var(--tag-color) should depend on var(--tag-bg) rather than var(--body-bg)
6 participants