-
Notifications
You must be signed in to change notification settings - Fork 841
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
Bug fix in contrastRatio function #3013
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tldr; The SASS contrast calculations do seem to be working correctly now in dark mode. 👍 But someone better at math than me should review the actual code.
Testing this locally by using the makeHighContrastColor()
for creating dark color contrast values, this PR does seem to fix the issues we were having before.
Examples:
Also, testing a color combination that fails minimum contrast without a calculation:
When debugging the SASS contrast values:
Before
// SASS Original contrast
contrastRatio($euiColorDanger, $euiColorLightShade): 3.45674
// SASS Calculated contrast
contrastRatio($euiColorDangerText, $euiColorLightShade): 4.50783
// JS Calculated contrast
5.4 !== 4.5
After
// SASS Original contrast
contrastRatio($euiColorDanger, $euiColorLightShade): 4.15215
// SASS Calculated contrast
contrastRatio($euiColorDangerText, $euiColorLightShade): 4.55297
// JS Calculated contrast
4.6 ~= 4.55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SASS code looks like it was ported correctly from the MDN JavaScript example, but I have one question on the first conditional
Co-Authored-By: Caroline Horn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM
I double checked this one as well. Merging! Thanks for the PR. |
Closes #2916
Fix of ContrastRatio function.
Checklist