[WEB-3540] dev: color picker component#6823
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces a new UI component, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CP as ColorPicker Component
participant CI as Color Input (hidden)
participant OC as onChange Callback
U->>CP: Clicks styled button
CP->>CI: Trigger click event (handleOnClick)
CI->>U: Opens native color picker
U->>CI: Selects new color
CI->>CP: Triggers onChange event
CP->>OC: Calls onChange callback with new color
sequenceDiagram
participant UI as UI Code
participant GI as getIconColor
participant EA as ensureAccessibleColors
participant Chroma as chroma-js
UI->>GI: Request icon color calculation
GI->>EA: Validate and adjust colors for accessibility
EA->>Chroma: Compute color contrasts & adjustments
Chroma-->>EA: Returns computed values
EA-->>GI: Adjusted foreground/background colors
GI-->>UI: Returns computed icon color
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/ui/src/color-picker/color-picker.tsx (3)
19-35: Consider showing visual feedback of the selected colorWhile the component functionality is correct, users might benefit from seeing the currently selected color. Consider adding a background color to the button based on the
valueprop.<button - className={`size-4 rounded-full cursor-pointer conical-gradient ${className}`} + className={`size-4 rounded-full cursor-pointer ${className}`} + style={{ backgroundColor: value }} onClick={handleOnClick} aria-label="Open color picker" />
24-24: Enhance accessibility with keyboard supportConsider adding keyboard support to allow users to open the color picker using keyboard navigation.
<button className={`size-4 rounded-full cursor-pointer conical-gradient ${className}`} onClick={handleOnClick} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleOnClick(); + } + }} aria-label="Open color picker" />
3-7: Add JSDoc comments to document the component propsAdding JSDoc comments would improve documentation for this component and help other developers understand its usage.
+/** + * ColorPicker component for selecting colors. + */ interface ColorPickerProps { + /** The current color value in hex format (e.g., "#FF0000") */ value: string; + /** Callback function that is called when the color changes */ onChange: (color: string) => void; + /** Optional className for additional styling */ className?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ui/src/color-picker/color-picker.tsx(1 hunks)packages/ui/src/color-picker/index.ts(1 hunks)packages/ui/src/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
packages/ui/src/color-picker/color-picker.tsx (1)
22-22: Verify custom CSS classesThe
size-4andconical-gradientCSS classes should be verified to ensure they exist in the project's CSS framework. Ifconical-gradientis a custom class, ensure it's properly defined and documented.packages/ui/src/color-picker/index.ts (1)
1-1: LGTM: Export pattern is correctThe barrel file pattern is correctly implemented, allowing for clean imports from the directory.
packages/ui/src/index.ts (1)
34-34: LGTM: Export is correctly addedThe new export follows the existing pattern and correctly exposes the color-picker module.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
web/helpers/theme.ts (3)
1-5: Consider exporting the HSLColor interface.The
HSLColorinterface is well-defined with appropriate comments, but it's not exported. If other components need to create or manipulate HSL colors directly, this interface should be exported to promote code reuse and type safety.-interface HSLColor { +export interface HSLColor { h: number; // hue (0-360) s: number; // saturation (0-100) l: number; // lightness (0-100) }
7-21: Consider adding input validation for HSL values.The
createBackgroundColorfunction handles different lightness ranges appropriately, but it doesn't validate that input values are within expected ranges (h: 0-360, s: 0-100, l: 0-100). This could lead to unexpected results with invalid inputs.export const createBackgroundColor = (hsl: HSLColor): string => { + // Ensure values are within valid ranges + const h = Math.max(0, Math.min(360, hsl.h)); + const s = Math.max(0, Math.min(100, hsl.s)); + const l = Math.max(0, Math.min(100, hsl.l)); + - if (hsl.l > 90) { + if (l > 90) { // Very light colors - increase opacity and darken slightly - return `hsla(${hsl.h}, ${hsl.s}%, ${Math.max(85, hsl.l - 10)}%, 0.25)`; + return `hsla(${h}, ${s}%, ${Math.max(85, l - 10)}%, 0.25)`; - } else if (hsl.l > 70) { + } else if (l > 70) { // Light colors - return `hsla(${hsl.h}, ${hsl.s}%, ${hsl.l - 5}%, 0.22)`; + return `hsla(${h}, ${s}%, ${l - 5}%, 0.22)`; - } else if (hsl.l > 50) { + } else if (l > 50) { // Medium colors - return `hsla(${hsl.h}, ${hsl.s}%, ${hsl.l}%, 0.18)`; + return `hsla(${h}, ${s}%, ${l}%, 0.18)`; } else { // Dark colors - increase lightness and opacity slightly - return `hsla(${hsl.h}, ${hsl.s}%, ${Math.min(hsl.l + 5, 50)}%, 0.15)`; + return `hsla(${h}, ${s}%, ${Math.min(l + 5, 50)}%, 0.15)`; } };
7-36: Consider adding unit tests for color manipulation functions.These color manipulation functions play an important role in maintaining visual consistency. Adding unit tests would help ensure they behave as expected across different input ranges and edge cases.
Example test structure:
// theme.test.ts import { createBackgroundColor, getIconColor } from './theme'; describe('createBackgroundColor', () => { test('handles very light colors correctly', () => { const result = createBackgroundColor({ h: 180, s: 50, l: 95 }); expect(result).toBe('hsla(180, 50%, 85%, 0.25)'); }); // Additional tests for other lightness ranges }); describe('getIconColor', () => { test('darkens very light colors for better contrast', () => { const result = getIconColor({ h: 180, s: 50, l: 90 }); expect(result).toBe('hsl(180, 60%, 31.5%)'); }); // Additional tests for other lightness ranges });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
web/helpers/theme.ts(1 hunks)web/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
web/package.json (1)
43-43: New Dependency Addition:chroma-js
The addition of"chroma-js": "^3.1.2"is appropriate for the newColorPickercomponent, as it will provide robust color manipulation functionalities. Please confirm that this version meets the compatibility requirements with the rest of the project's dependencies, and consider verifying if any peer dependency warnings are generated during installation.web/helpers/theme.ts (1)
23-36:Details
❓ Verification inconclusive
Review saturation calculation for dark colors.
In the dark colors case, you're calculating saturation as
Math.min(hsl.l * 2, 85)which uses the lightness value to modify saturation. This might be intentional for a specific visual effect, but it's unusual to couple these two properties this way. Consider if this is the desired behavior.
🌐 Web query:
What is the relationship between saturation and lightness in HSL color model?💡 Result:
In the HSL color model, saturation and lightness are interdependent properties that together determine a color's intensity and brightness. Here's a detailed breakdown of their relationship:
Core Interaction
- Saturation (0-100%) controls color purity (0% = gray, 100% = fully saturated)
- Lightness (0-100%) controls brightness (0% = black, 100% = white)
- Purest colors occur at 50% lightness with 100% saturation[2][5]
Key Observations
Lightness dominance:
- At 0% or 100% lightness, colors become pure black/white regardless of saturation[2][5]
- Only at 50% lightness can colors achieve full saturation (100%)[4]
Saturation limitations:
- Maximum saturation decreases as lightness moves away from 50%
- Colors become inherently less intense when:
- Lightness >50% (tints with added white)
- Lightness <50% (shades with added black)[3][5]
Practical manipulation:
- Tints: Increase lightness (>50%) → reduce perceived saturation
- Shades: Decrease lightness (<50%) → reduce perceived saturation
- Tones: Mix with gray by lowering saturation at any lightness[2][7]
Visual Comparison
Lightness Level Pure Saturation Effect Visual Result 0% No effect (always black) Black 25% Maximum muted saturation Dark shaded color 50% Full possible saturation Most vibrant hue 75% Maximum pastel saturation Light tinted color 100% No effect (always white) White This relationship allows precise control in design workflows - adjusting lightness creates natural-looking tints/shadows while preserving the base hue[8][16]. In CSS, colors like
hsl(200, 100%, 50%)represent the pure cyan at mid-lightness, whilehsl(200, 100%, 20%)would show a darker, equally saturated navy blue[10][13].Citations:
- 1: https://en.wikipedia.org/wiki/HSL_and_HSV
- 2: https://itequia.design/en/hsl-model-what-is-and-how-does-it-differ-from-rgb-or-hex-2/
- 3: https://forum.freecodecamp.org/t/the-basic-principles-of-web-design-a-guide-to-using-hsl-what-is-it-and-is-it-better-than-rgb/326663
- 4: https://programmingdesignsystems.com/color/color-models-and-color-spaces/index.html
- 5: https://www.tobiamontanari.com/hsl-and-hsv-explained-which-color-model-should-you-use/
- 6: https://giggster.com/guide/basics/hue-saturation-lightness/
- 7: https://data.europa.eu/apps/data-visualisation-guide/describing-colours-hsl
- 8: https://www.merixstudio.com/blog/hsl-color-model
- 9: https://www.antennahouse.com/hubfs/xsl-fo-sample/color/color-hsl-1.pdf
- 10: https://www.w3schools.com/html/html_colors_hsl.asp
- 11: https://cloudfour.com/thinks/hsl-a-color-format-for-humans/
- 12: https://www.w3schools.com/colors/colors_hsl.asp
- 13: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/hsl
- 14: https://www.helpscout.com/blog/hsl-color-in-css/
- 15: https://global.canon/en/imaging/picturestyle/editor/matters05.html
- 16: https://www.coconut.co/articles/hsl-a-painters-dream
- 17: https://www.khanacademy.org/video/color-3
- 18: https://vanseodesign.com/web-design/hue-saturation-and-lightness/
- 19: https://okbinteractive.studio/en/insights/why-use-hsl-for-working-with-colors.html
Action needed: Verify the saturation adjustment for dark colors
- In
web/helpers/theme.ts, the branch for very dark colors (whenhsl.l < 30) computes saturation asMath.min(hsl.l * 2, 85).- This approach couples saturation directly with lightness—a pattern that differs from standard HSL usage, where saturation and lightness are typically adjusted independently.
- Given that in the HSL model maximum color intensity is achieved at around 50% lightness, using a calculation based strictly on lightness might lead to less vivid colors than intended if the original
hsl.swas higher.- Please confirm whether this behavior is a deliberate visual effect. If it is intentional, consider adding a clarifying comment. Otherwise, evaluate if the original or a different adjustment for saturation might better preserve the intended color appearance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/helpers/theme.tsx (2)
15-65: Function logic is sound but has room for optimizationThe
ensureAccessibleColorsfunction has a solid approach for ensuring color contrast, but there are some optimization opportunities:
- The algorithm always uses the original
fgandbgfor determining whether to make colors lighter/darker, even after adjustments have been made.- It adjusts both colors in each iteration before checking contrast, which may lead to unnecessary adjustments.
Consider refactoring to use the adjusted colors for comparison in each iteration:
- if (fg.luminance() > bg.luminance()) { + if (adjustedFg.luminance() > adjustedBg.luminance()) {Also, consider exporting this function since it provides useful functionality that might be needed elsewhere in the application.
67-86: Missing handling for middle lightness rangeThe
createBackgroundColorfunction handles adjustments for very light colors (l > 90 and l > 70) and very dark colors (l < 30), but there's no explicit handling for the middle range (30-70).Consider adding a comment explaining the default behavior for the middle range or adding explicit handling if needed:
} else if (hsl.l < 30) { backgroundColor = baseColor.brighten(0.5).alpha(resolvedTheme === "dark" ? 0.22 : 0.12); + } else { + // Middle range (30-70) uses the base opacity without brightness adjustments }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/helpers/theme.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
web/helpers/theme.tsx (2)
1-14: Well-structured interfaces with clear documentationThe interfaces are properly defined with helpful comments explaining the range and purpose of each property. This facilitates understanding of the data structures used throughout the file.
1-100:Details
✅ Verification successful
Verify WCAG color contrast calculations
The implementation uses chroma.js for calculating color contrast, but it's important to ensure the calculations meet the latest WCAG standards for accessibility.
🌐 Web query:
Is chroma.js contrast calculation compliant with WCAG 2.1 standards?💡 Result:
Chroma.js provides a WCAG 2.1-compliant contrast calculation method that aligns with the standard’s requirements for measuring luminance ratios between text and background colors. The library implements the formula
(Y_max + 0.05) / (Y_min + 0.05)[9], which matches the WCAG 2.1 specification for contrast ratios[2][3][6]. This method is widely used for accessibility validation, though it has documented limitations in perceptual accuracy compared to newer models like APCA[1][5].Key Compliance Points:
- Threshold Adherence: Chroma.js accurately calculates the 4.5:1 minimum ratio for standard text and 3:1 for large text (≥18pt or ≥14pt bold), as required by WCAG 2.1 Level AA[6][10][16].
- Formula Implementation: The calculation includes a 0.05 offset for both foreground and background luminance to simulate real-world viewing conditions (e.g., screen glare)[5][9].
- Non-Text Contrast Support: While not explicitly mentioned in Chroma.js documentation, WCAG 2.1’s 3:1 ratio for UI components (buttons, icons) can be verified using the same method[2][20].
Limitations to Note:
- Perceptual Inaccuracy: WCAG 2.1’s linear model may fail to detect insufficient contrast in dark-mode interfaces or low-saturation colors[1][5]. For example, white text on a dark blue background might pass WCAG 2.1 but remain hard to read[14].
- Alternatives in Development: Chroma.js also offers APCA contrast calculations[9], which address these perceptual issues and align with the upcoming WCAG 3.0 draft[1][5].
For compliance validation, Chroma.js’s WCAG 2.1 method remains a valid tool but should be supplemented with manual checks or APCA-based evaluations for complex color schemes[5][15].
Citations:
- 1: https://www.wildbit.com/blog/accessible-palette-stop-using-hsl-for-color-systems.html
- 2: https://webaim.org/articles/contrast/
- 3: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html
- 4: w3c/wcag#623
- 5: https://colorjs.io/docs/contrast
- 6: https://www.washington.edu/accesstech/checklist/contrast/
- 7: https://my.willamette.edu/site/digital-accessibility/guidelines/color-contrast
- 8: w3c/csswg-drafts#7357
- 9: https://gka.github.io/chroma.js/
- 10: https://accessibleweb.com/color-contrast-checker/
- 11: https://accessiblepalette.com
- 12: https://www.w3.org/WAI/WCAG22/Understanding/contrast-minimum
- 13: https://help.nimbleams.com/help/live/enablement-and-validation-of-wcag-compliance
- 14: https://webaim.org/resources/contrastchecker/
- 15: https://www.maplibrary.org/1400/adaptive-color-schemes-for-interactive-maps/
- 16: https://adasitecompliance.com/ada-color-contrast-checker/
- 17: https://brand.umich.edu/design-resources/accessibility/
- 18: https://dequeuniversity.com/rules/axe/4.9/color-contrast
- 19: https://www.w3.org/WAI/test-evaluate/tools/list/
- 20: https://dequeuniversity.com/resources/wcag2.1/1.4.11-non-text-contrast
WCAG 2.1 Contrast Compliance Confirmed
After reviewing the implementation, the use of Chroma.js for calculating contrast is compliant with WCAG 2.1 standards. The code correctly applies the formula
(Y_max + 0.05) / (Y_min + 0.05), meeting the necessary thresholds for both text and UI components. While the library has known perceptual limitations compared to methods like APCA, these do not affect its current compliance.
- In
ensureAccessibleColors: The contrast adjustment loop effectively uses Chroma.js to meet the specified WCAG requirements.- Thresholds: The default target contrast of 4.5 (for WCAG AAA) and 3 (for UI components) are appropriate.
* dev: color picker component added * chore: helper function added * chore: code refactor * chore: code refactor * chore: code refactor * chore: code refactor
3 participants
Description
This PR introduces a color picker component.
Type of Change
References
[WEB-3540]
Summary by CodeRabbit
New Features
Dependencies