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

feat: contrast util with yiq calculator #3652

Merged
merged 7 commits into from
Nov 7, 2022

Conversation

Ornanovitch
Copy link
Contributor

@Ornanovitch Ornanovitch commented Sep 25, 2022

Needed for #3653

Changes proposed in this pull request:

  • The isDark utility converts a hex color to rgb, and then calcul a YIQ value in order to get the appropriate contrast value (is it dark or is it light?)
  • In order to use this new snippet without any pain, I created two new color variables which aren't depending on the dark/light mode: @text-on-light and @text-on-dark

See #3653 for example

See https://www.w3.org/TR/AERT/#color-contrast for references

Reviewers should focus on:

I don't know... Is the name getContrast ok regarding your naming conventions?

Screenshot

see #3653

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 are not appropriate here.

@Ornanovitch Ornanovitch requested a review from a team as a code owner September 25, 2022 16:26
Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

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

Ooh, you moved this here! Makes sense.

I left a comment about exporting an isDark util so we don't need to compare against 128 in our code in your draft PR. Could we do that here?

@Ornanovitch
Copy link
Contributor Author

I left a comment about exporting an isDark util so we don't need to compare against 128 in our code in your draft PR. Could we do that here?

You're right! I'll do it asap :)

@Ornanovitch
Copy link
Contributor Author

@davwheat It seems a lot better, thanks for your advice!

@davwheat davwheat requested a review from SychO9 October 21, 2022 12:52
@imorland imorland merged commit fccc3e2 into flarum:main Nov 7, 2022
@imorland imorland added type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) type/extensibility labels Nov 7, 2022
@imorland imorland added this to the 1.6 milestone Nov 7, 2022
@Ornanovitch Ornanovitch deleted the contrast-util branch November 7, 2022 11:24
@Ornanovitch
Copy link
Contributor Author

:)

@luceos luceos mentioned this pull request Nov 10, 2022
@iPurpl3x
Copy link
Contributor

@Ornanovitch, @imorland, @davwheat isDark throws an error and the app crashes if a color is set to NULL in the database…

@SychO9
Copy link
Member

SychO9 commented Mar 16, 2023

please open a bug report with specifics.

@iPurpl3x
Copy link
Contributor

Here you go @SychO9 #3764

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) type/extensibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants