Skip to content

feat: contrast util with yiq calculator#3652

Merged
imorland merged 7 commits into
flarum:mainfrom
ornanovitch:contrast-util
Nov 7, 2022
Merged

feat: contrast util with yiq calculator#3652
imorland merged 7 commits into
flarum:mainfrom
ornanovitch:contrast-util

Conversation

@ornanovitch
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@davwheat davwheat requested a review from SychO9 October 21, 2022 12:52
Comment thread framework/core/js/src/common/utils/isDark.ts Outdated
Comment thread framework/core/js/src/common/utils/isDark.ts Outdated
@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
Copy Markdown
Contributor Author

:)

@luceos luceos mentioned this pull request Nov 10, 2022
@iPurpl3x
Copy link
Copy Markdown
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
Copy Markdown
Contributor

SychO9 commented Mar 16, 2023

please open a bug report with specifics.

@iPurpl3x
Copy link
Copy Markdown
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