feat(eds-color-palette-generator): 4246 add optional second color input to palette generator #4247
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR successfully implements multi-anchor color interpolation for the EDS color palette generator, enabling users to define multiple color anchors at specific palette steps and interpolate smoothly between them. The implementation maintains backwards compatibility with the existing single-color format.
Key changes:
- Added support for multiple color anchors with associated palette step positions
- Implemented OKLCH-based interpolation logic with Gaussian chroma adjustment
- Extended UI to allow users to add, edit, and remove color anchors
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
packages/eds-tokens/palette-config.json |
Updated configuration with anchor format examples for Moss Green and North sea colors |
apps/eds-color-palette-generator/src/utils/configurationUtils.ts |
Added backwards-compatible support for both legacy single value and new anchor format in export/download functions |
apps/eds-color-palette-generator/src/utils/compare.ts |
Extended comparison logic to handle anchor arrays with deep comparison of step and value properties |
apps/eds-color-palette-generator/src/utils/compare.test.ts |
Added comprehensive test suite for anchor comparison including edge cases and mixed formats |
apps/eds-color-palette-generator/src/utils/color.ts |
Implemented new interpolation function with helper utilities, refactored existing logic for code reuse |
apps/eds-color-palette-generator/src/utils/color.test.ts |
Added extensive test coverage for helper functions and interpolation behavior including multi-anchor scenarios |
apps/eds-color-palette-generator/src/types.ts |
Extended type system to support optional anchor arrays while maintaining backwards compatibility |
apps/eds-color-palette-generator/src/config/palette.ts |
Updated default palette configuration with anchor examples matching JSON config |
apps/eds-color-palette-generator/src/components/ColorScale.tsx |
Significant UI enhancement to support anchor management with add/remove controls and validation |
apps/eds-color-palette-generator/src/app/page.tsx |
Updated parent state management and memoization to handle both legacy and anchor-based color definitions |
apps/eds-color-palette-generator/plan/multi-color-interpolation.plan.md |
Added comprehensive implementation plan and grading rubric for feature development |
apps/eds-color-palette-generator/src/utils/configurationUtils.ts
Outdated
Show resolved
Hide resolved
apps/eds-color-palette-generator/src/utils/configurationUtils.ts
Outdated
Show resolved
Hide resolved
apps/eds-color-palette-generator/src/utils/configurationUtils.ts
Outdated
Show resolved
Hide resolved
|
@copilot please implement all your suggestions |
|
@torleifhalseth I've opened a new pull request, #4248, to work on those changes. Once the pull request is ready, I'll request review from you. |
…n for color palette generation - Introduced support for multiple color anchors in the color palette generator, allowing interpolation between defined anchor colors. - Updated type definitions to accommodate new `ColorAnchor` structure. - Enhanced configuration files to demonstrate the new anchor format while maintaining backward compatibility. - Developed core interpolation logic to generate color scales based on multiple anchors, including Gaussian chroma adjustments. - Updated UI components to manage and display anchors, including adding and removing anchors dynamically. - Added unit tests to ensure functionality and edge cases for the new interpolation feature.
… enhance color formatting utilities - Introduced the AnchorColorInput component for managing color anchors with input validation and color picking functionality. - Enhanced color formatting utilities with new functions: formatColorAsString for converting Color objects to string formats (OKLCH and HEX) and getFallbackColor for providing default color values. - Updated generateColorScale and related functions to utilize the new color formatting utilities, ensuring consistent output across different formats. - Added comprehensive unit tests for new utilities and the AnchorColorInput component to ensure reliability and correctness.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…improve anchor management - Updated the ColorScale and AnchorColorInput components to utilize the new anchors array instead of the legacy value field. - Simplified color input handling by introducing a helper function to ensure either anchors or value is provided. - Enhanced validation to alert users when all anchor steps are in use, preventing further additions without removal. - Improved key generation for anchor inputs to ensure uniqueness based on value and step.
…4248) * Initial plan * refactor: implement code review suggestions for better maintainability and UX - Extract magic color constants to constants.ts file - Add JSDoc documentation for AnchorColorInput component - Add helper function isStepAlreadyUsed to reduce code duplication - Add input validation for anchor step ranges (1-15) - Improve error message guidance in configurationUtils - Add comments explaining intentional callback exclusion in memo functions - Add comment explaining legacy value field removal - Remove inconsistent semicolons in JSX - Add notes about window.alert needing future UX improvements - Update compare.ts comment for clarity Co-authored-by: torleifhalseth <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: torleifhalseth <[email protected]>
…ptimization - Add support for color anchors (interpolation between multiple colors) - CLI now accepts both legacy 'value' and new 'anchors' format - Export optimization: single-anchor colors converted to simpler 'value' format - Fix TypeScript errors in generate-palette-contrast-report script - Add comprehensive test coverage (10 new tests) - Maintain full backward compatibility with existing configurations
- Update About page with interpolation explanation - Add visual comparison of single value vs multiple anchors - Update README.md with new feature highlights - Update CLI README with both configuration formats - Show how colors interpolate between anchor points
…use mid-step anchors - Change anchors from steps 1 & 15 to steps 6 & 9 - Use OKLCH values: step 6 (0.5915 0.0731 184.63), step 9 (0.4973 0.084851 204.553) - Update all 15 color values with actual generated colors from test snapshot - Add test case for mid-step anchor configuration - Highlight anchor steps 6 and 9 with blue borders in visualization
millus
force-pushed
the
feat/4246-add-optional-second-color-input-to-palette-generator
branch
from
6e7cda8 to
c13b803
Compare
… tests - Fix lint warning by removing unused variable - Clarify that single-anchor arrays are edge cases, not normal UI flow - Remove 2 redundant tests that were just combining core functionality - Update test comments to reflect actual usage patterns - Run prettier formatting on all files
millus
requested review from
Chibuzor-Nwemambu,
millus and
pomfrida
as code owners
| window.alert( | ||
| 'Step value already used by another anchor. Please choose a unique step.', | ||
| ) | ||
| return | ||
| } | ||
| newAnchors[index] = { ...newAnchors[index], step: newValue } | ||
| } | ||
| onChangeAnchors(newAnchors) | ||
| } | ||
|
|
||
| const handleAddAnchor = () => { | ||
| if (!anchors || !onChangeAnchors) return | ||
| // Find an available step (one that's not already used) | ||
| const usedSteps = anchors.map((a) => a.step) | ||
|
|
||
| // Check if all 15 steps are already used | ||
| if (usedSteps.length >= 15) { | ||
| // Note: Using alert here. Consider replacing with a toast notification | ||
| // or inline error message for better UX in a future update | ||
| window.alert( | ||
| 'All 15 steps are already in use. Remove an anchor before adding a new one.', | ||
| ) | ||
| return |
There was a problem hiding this comment.
Using window.alert() for error messages is not user-friendly and blocks the UI. Consider using inline error messages, toasts, or other non-blocking notification mechanisms. This applies to both error cases here:
- Duplicate step validation (line 392)
- Maximum anchors validation (line 411)
The comments on lines 390-391 and 409-410 already acknowledge this should be improved, but it's important to address for better UX.
| export type ColorDefinition = { | ||
| name: string | ||
| value: string | ||
| // Support both legacy single value and new anchor array | ||
| value?: string // legacy single color | ||
| anchors?: ColorAnchor[] // new multi-color with steps | ||
| } |
There was a problem hiding this comment.
The ColorDefinition type allows both value and anchors to be optional, which could result in a color definition with neither field. While there's runtime validation in getColorInput functions throughout the codebase, the type system should enforce that at least one is required. Consider using a discriminated union:
export type ColorDefinition =
| { name: string; value: string }
| { name: string; anchors: ColorAnchor[] }This would make invalid states unrepresentable at compile time and eliminate the need for runtime validation in many places.
| {Array.from({ length: 15 }, (_, i) => i + 1).map((step) => { | ||
| // Find if this step is used by another anchor | ||
| const isUsed = isStepAlreadyUsed(anchors, step, index) | ||
| return ( | ||
| <option key={step} value={step} disabled={isUsed}> | ||
| Step {step} | ||
| {isUsed ? ' (used)' : ''} | ||
| </option> | ||
| ) | ||
| })} |
There was a problem hiding this comment.
The step dropdown options show disabled states for used steps, but the text "(used)" is embedded in the option value. Screen readers may not convey this clearly. Consider adding a more explicit aria-label or using aria-describedby to provide better context about why an option is disabled.
Example:
<option
key={step}
value={step}
disabled={isUsed}
aria-label={isUsed ? `Step ${step} - already in use` : `Step ${step}`}
>
Step {step}{isUsed ? ' (used)' : ''}
</option>| let newStep = 8 // Default to middle step | ||
| for (let i = 1; i <= 15; i++) { | ||
| if (!usedSteps.includes(i)) { | ||
| newStep = i | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
The default step selection logic uses hardcoded value 8 as the middle step, then falls back to searching from 1. Consider extracting this logic to a helper function like findAvailableStep(usedSteps: number[], preferredStep: number = 8): number for better maintainability and testability. This would also make the intent clearer and allow easier modification if the preferred logic changes.
| if (debounceRef.current) { | ||
| window.clearTimeout(debounceRef.current) | ||
| } | ||
| debounceRef.current = window.setTimeout(() => { | ||
| onUpdateAnchor(index, 'value', value.trim()) | ||
| }, 250) |
There was a problem hiding this comment.
The debounce mechanism uses window.setTimeout and clears the previous timeout, but doesn't clean up the timeout on component unmount. If the component unmounts before the debounced callback fires, this could cause a memory leak or attempt to update unmounted component state. Consider adding cleanup in a useEffect return or using a custom debounce hook that handles cleanup automatically.
torleifhalseth
deleted the
feat/4246-add-optional-second-color-input-to-palette-generator
branch
This pull request implements multi-anchor color interpolation for the EDS color palette generator, allowing users to define multiple color anchors per palette color and interpolate smoothly between them. The update includes changes to the type system, configuration, interpolation logic, UI components, and parent state management, while maintaining backwards compatibility with legacy single-color palettes.
Multi-anchor interpolation and UI support:
ColorDefinitiontype, enabling each color to have an array of anchor points with associated palette steps.ColorScale.tsxto display and manage anchors, allowing users to add, edit, and remove anchors, with validation for step uniqueness and color format. [1] [2] [3] [4]Core logic and state management:
generateColorScaleWithInterpolation, using color.js to interpolate between anchors in OKLCH space, and updated palette generation to delegate to this function when anchors are present.page.tsxto normalize color data, handle anchor updates, and ensure memoization works for both legacy and anchor-based palettes. [1] [2] [3]Backwards compatibility and configuration:
Testing and documentation: