-
Notifications
You must be signed in to change notification settings - Fork 843
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
Draft PR: Working on issue #7824 #7903
base: main
Are you sure you want to change the base?
Conversation
💚 CLA has been signed |
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
Hi @cee-chen, I have made this PR Will be starting with the direction @daniel-rhoades mentioned to use the makeHighContrastColor function in the code. Would it be a good start? |
@Arup-Chauhan My preference would be to skip I think the simplest path forward would be to DRY out the shared color contrast logic between EuiAvatar and EuiBadge to an exported utility (which we can locate in |
so we are looking at a shared utility for the color contrast logic and place it in src/services/. This will eliminate duplicate implementations in EuiAvatar and EuiBadge. By using this utility, EuiAvatar can also throw a console warning if the color contrast is insufficient. Is this a correct way? |
That's correct! |
Cool, thanks for the feedback, I will start doing it and let you know how it goes, have a good day! |
@cee-chen Hi, just wanted to update you that I am in the middle of some summer break commitments from college, will resume working on this soon asap. Hope that works! |
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
@cee-chen Hi Cee, wanted to share that I have kind of coded a logic using the constrst utility, will push the changes in this draft repo for your review by the end of this week. Thanks for the patience, the college semester course load is a little tough this time so spending my major amount there! |
No worries at all. This isn't a high priority task, so take your time! |
Hi @cee-chen , I hope you're doing well. I noticed that the issue is marked as closed. I wasn’t able to update you earlier due to some urgent academic commitments. If it’s still possible, I would love to continue working on this. I've made some progress on creating the shared color contrast utility to eliminate duplicate implementations in components like /**
* Calculates the contrast ratio between two colors.
* @param {string} foreground - The foreground color in hexadecimal format.
* @param {string} background - The background color in hexadecimal format.
* @returns {number} - The contrast ratio between the two colors.
*/
function calculateContrast(foreground: string, background: string): number {
const lum1 = getLuminance(foreground);
const lum2 = getLuminance(background);
const brightest = Math.max(lum1, lum2);
const darkest = Math.min(lum1, lum2);
return (brightest + 0.05) / (darkest + 0.05);
} This utility will help ensure the colors meet accessibility standards. Let me know if I can proceed with further refinements or if there are any changes you'd like me to incorporate. Looking forward to your response! |
There isn't a need for a custom eui/packages/eui/src/components/badge/color_utils.ts Lines 68 to 70 in 5c40315
You should be able to simply re-use the existing util and EuiBadge's logic to throw an error when color contrast is too low: eui/packages/eui/src/components/badge/badge.tsx Lines 145 to 153 in 7fac1ce
My preferred approach would be moving the |
Hi @cee-chen, thanks for being patient as work through on this issue and thanks for your feedback I have made the following changes and pushed it in the draft PR too. Replaced the existing contrast-checking logic in
import chroma from 'chroma-js';
export const getColorContrast = (textColor: string, backgroundColor: string): number => {
return chroma.contrast(textColor, backgroundColor);
};
export const warnIfContrastBelowMin = (textColor: string, backgroundColor: string, wcagContrastMin: number): void => {
const contrastRatio = getColorContrast(textColor, backgroundColor);
if (contrastRatio < wcagContrastMin) {
console.warn(
`Warning: ${backgroundColor} background with ${textColor} text has a low contrast ratio of ${contrastRatio.toFixed(
2
)}. Should be above ${wcagContrastMin}.`
);
}
};
Original code: const contrastRatio = getColorContrast(textColor, color);
if (contrastRatio < wcagContrastMin) {
console.warn(
`Warning: ${color} badge has a low contrast of ${contrastRatio.toFixed(
2
)}. Should be above ${wcagContrastMin}.`
);
} Replaced with: import { warnIfContrastBelowMin } from '../../../services/color/contrast';
warnIfContrastBelowMin(textColor, color, wcagContrastMin); This change centralizes the contrast-checking logic, leveraging Also, I would like to know how to check the changes and verify them |
Signed-off-by: Arup Chauhan <[email protected]>
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
Working on issue #7824