Enhance color palette generation with saturation curve options#3942
Enhance color palette generation with saturation curve options#3942
Conversation
- Updated GeneratePaletteOptions to include saturationCurve, saturationThreshold, and saturationFloor for improved saturation adjustments. - Refactored adjustSaturation function to utilize the new saturation curve logic. - Modified color tint tests to reflect new expected values based on saturation adjustments. - Added tests to ensure saturation curve behavior respects thresholds and custom floor values.
✅ PR Description Validation PassedAll required sections are properly filled out:
Your PR is good for review! 🚀 This validation ensures all sections from the PR template are properly filled. |
lidord-wix
left a comment
lidord-wix
left a comment
There was a problem hiding this comment.
I added some comments, also maybe worth going over it together to make sure I understand the changes
| adjustSaturation?: boolean; | ||
| /** Array of saturation adjustments to apply on the color's tints array (from darkest to lightest). | ||
| * The 'adjustSaturation' option must be true */ | ||
| saturationLevels?: number[]; |
There was a problem hiding this comment.
what if someone is passing saturationLevels?
it seems like a breaking change..
There was a problem hiding this comment.
I asked Alexey and Yulia, we do want to change the logic entirely.
If I'm not mistaken, at Wix this logic is passed only via the private, so it should break our modules, but it is breaking change for public users. what do we want to do? still allow the public users use the old logic, or make them adjust to the new logic?
| const sliced = tints.slice(start, end); | ||
|
|
||
| const adjusted = options?.adjustSaturation && adjustSaturation(sliced, color, options?.saturationLevels); | ||
| const adjusted = options?.adjustSaturation && adjustSaturationWithCurve(sliced, color, options); |
There was a problem hiding this comment.
The function expect to get CurveOptions but gets GeneratePaletteOptions
| const sliced = tints.slice(start, end); | ||
|
|
||
| const adjusted = options?.adjustSaturation && adjustSaturation(sliced, color, options?.saturationLevels); | ||
| const adjusted = options?.adjustSaturation && adjustSaturationWithCurve(sliced, color, options); |
There was a problem hiding this comment.
When adjustSaturation is false (or not provided), adjustSaturationWithCurve is never called.
However, defaultPaletteOptions sets adjustSaturation: true, so calling generateColorPalette without options always applies the curve. This is a behavior change from before where the old fallback adjustSaturation function only modified colors when lightness > 80 && saturation > 60. Now the curve is applied to all colors with baseSaturation > 50.
This means previously unaffected palettes (e.g., colors with lightness < 80 but saturation > 50) will now get
modified. This widens the blast radius of the change.
There was a problem hiding this comment.
Yes, we want the saturation curve to be applied as default, and only if the user decides to - they can still turn it off by sending false in the options. The curve also wont be applied if the base saturation is below 50 - it's part of the logic.
| const hsl = Color(hex).hsl(); | ||
| const distance = Math.abs(i - baseIndex); | ||
| const percentage = curve[Math.min(distance, curve.length - 1)]; | ||
| const newSaturation = Math.max(floor, Math.ceil(baseSaturation * percentage)); |
There was a problem hiding this comment.
why using Math.ceil here?
There was a problem hiding this comment.
please also cover the new logic where the addDarkestTints: true
…tions - Changed the saturation adjustment calculation from ceil to round
2 participants
Description
Changelog
Colors: ColorPalette new Lightness and Saturation logic
Additional info
Ticket 4308