-
Notifications
You must be signed in to change notification settings - Fork 0
🧹 [Code Health] Simplify adjustAccentColor function in color.ts #50
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||
| import { colord, extend } from "colord"; | ||||||||||
| import { colord, extend, type Colord } from "colord"; | ||||||||||
| import mixPlugin from "colord/plugins/mix"; | ||||||||||
| import namesPlugin from "colord/plugins/names"; | ||||||||||
| import a11yPlugin from "colord/plugins/a11y"; | ||||||||||
|
|
@@ -21,13 +21,10 @@ const TARGET_MIN_LIGHTNESS = 45; | |||||||||
| const TARGET_MAX_LIGHTNESS = 85; | ||||||||||
| const HOVER_LIGHTEN_AMOUNT = 0.1; | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Adjusts the given color to be suitable for use as an accent color in a dark theme. | ||||||||||
| * Ensures sufficient saturation and appropriate lightness. | ||||||||||
| * @param color Hex string or RGB object/array | ||||||||||
| */ | ||||||||||
| export function adjustAccentColor(color: string | [number, number, number] | { r: number; g: number; b: number }): ColorResult { | ||||||||||
| let c; | ||||||||||
| type ColorInput = string | [number, number, number] | { r: number; g: number; b: number }; | ||||||||||
|
|
||||||||||
| function parseColor(color: ColorInput): Colord { | ||||||||||
| let c: Colord; | ||||||||||
|
|
||||||||||
| if (Array.isArray(color)) { | ||||||||||
| c = colord({ r: color[0], g: color[1], b: color[2] }); | ||||||||||
|
|
@@ -37,27 +34,43 @@ export function adjustAccentColor(color: string | [number, number, number] | { r | |||||||||
|
|
||||||||||
| // Ensure valid color, fallback to default blue if invalid | ||||||||||
| if (!c.isValid()) { | ||||||||||
| c = colord(DEFAULT_ACCENT_COLOR); | ||||||||||
| return colord(DEFAULT_ACCENT_COLOR); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Adjust saturation: if too low, saturate | ||||||||||
| return c; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| function ensureSaturation(c: Colord): Colord { | ||||||||||
| const s = c.toHsl().s; | ||||||||||
| if (s < MIN_SATURATION_FOR_HIGH_BOOST) { | ||||||||||
| c = c.saturate(HIGH_SATURATION_BOOST); | ||||||||||
| return c.saturate(HIGH_SATURATION_BOOST); | ||||||||||
| } else if (s < MIN_SATURATION_FOR_LOW_BOOST) { | ||||||||||
| c = c.saturate(LOW_SATURATION_BOOST); | ||||||||||
| return c.saturate(LOW_SATURATION_BOOST); | ||||||||||
| } | ||||||||||
| return c; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Adjust lightness for dark mode context | ||||||||||
| // Should be bright enough to glow, but not white | ||||||||||
| function adjustLightness(c: Colord): Colord { | ||||||||||
| const l = c.toHsl().l; | ||||||||||
| if (l < TARGET_MIN_LIGHTNESS) { | ||||||||||
| // Lift to at least TARGET_MIN_LIGHTNESS | ||||||||||
| c = c.lighten((TARGET_MIN_LIGHTNESS - l) / 100); | ||||||||||
| return c.lighten((TARGET_MIN_LIGHTNESS - l) / 100); | ||||||||||
| } else if (l > TARGET_MAX_LIGHTNESS) { | ||||||||||
| // Dim to at most TARGET_MAX_LIGHTNESS | ||||||||||
| c = c.darken((l - TARGET_MAX_LIGHTNESS) / 100); | ||||||||||
| return c.darken((l - TARGET_MAX_LIGHTNESS) / 100); | ||||||||||
| } | ||||||||||
| return c; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Adjusts the given color to be suitable for use as an accent color in a dark theme. | ||||||||||
| * Ensures sufficient saturation and appropriate lightness. | ||||||||||
| * @param color Hex string or RGB object/array | ||||||||||
| */ | ||||||||||
| export function adjustAccentColor(color: string | [number, number, number] | { r: number; g: number; b: number }): ColorResult { | ||||||||||
| let c = parseColor(color); | ||||||||||
| c = ensureSaturation(c); | ||||||||||
| c = adjustLightness(c); | ||||||||||
|
Comment on lines
+71
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The refactoring into helper functions is great. To make the pipeline nature of the transformations even clearer, you could compose these function calls into a single expression. This also allows you to declare
Suggested change
|
||||||||||
|
|
||||||||||
| const hex = c.toHex(); | ||||||||||
| const rgb = c.toRgb(); | ||||||||||
|
|
||||||||||
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.
To maintain consistency with the newly introduced
ColorInputtype alias, it's recommended to use it in the function signature here as well. This avoids repeating a complex type and improves readability.