Added Colorblind-Accessible Color Palettes and Opacity Control for Isochrones#344
Added Colorblind-Accessible Color Palettes and Opacity Control for Isochrones#344ritgit24 wants to merge 2 commits intovalhalla:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds colorblind-accessible color palette options and an opacity slider for isochrone visualizations, addressing issue #283. It introduces viridis and magma color palettes as alternatives to the original red-green gradient, and allows users to control the transparency of isochrone polygons through a slider in the Isochrones Settings panel.
Changes:
- New utility module (
isochrone-colors.ts) with color interpolation functions for viridis, magma, and default palettes, along with comprehensive unit tests. - Store and UI updates to add
colorPaletteandopacitystate, a dropdown selector for palettes, and an opacity slider in the Isochrones Settings panel. - Updated
isochrone-polygons.tsxto dynamically compute fill colors based on the selected palette and apply the opacity setting.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/isochrone-colors.ts |
New file defining color palette utilities with interpolation functions for viridis, magma, and default palettes |
src/utils/isochrone-colors.spec.ts |
New tests for color palette utilities |
src/stores/isochrones-store.ts |
Added colorPalette and opacity state with actions updateColorPalette and updateOpacity |
src/components/map/parts/isochrone-polygons.tsx |
Dynamic fill color computation per palette and opacity application |
src/components/map/parts/isochrone-polygons.spec.tsx |
Updated mock data with contour values and new store fields |
src/components/isochrones/waypoints.tsx |
Added Color Palette dropdown and Opacity slider to settings panel |
src/components/isochrones/waypoints.spec.tsx |
Added mocks for new store fields and actions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getViridisColor(value: number): string { | ||
| const v = Math.max(0, Math.min(1, value)); | ||
|
|
||
| //5 key stops | ||
| const colors: [number, string][] = [ | ||
| [0.0, '#440154'], // dark purple | ||
| [0.25, '#31688e'], // blue | ||
| [0.5, '#35b779'], // green | ||
| [0.75, '#90d743'], // yellow-green | ||
| [1.0, '#fde724'], // yellow | ||
| ]; | ||
|
|
||
|
|
||
| let lower: [number, string] = colors[0]!; | ||
| let upper: [number, string] = colors[colors.length - 1]!; | ||
|
|
||
| for (let i = 0; i < colors.length - 1; i++) { | ||
| const current = colors[i]; | ||
| const next = colors[i + 1]; | ||
| if (current && next && v >= current[0] && v <= next[0]) { | ||
| lower = current; | ||
| upper = next; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const range = upper[0] - lower[0]; | ||
| const t = range === 0 ? 0 : (v - lower[0]) / range; | ||
|
|
||
| const color1 = hexToRgb(lower[1]); | ||
| const color2 = hexToRgb(upper[1]); | ||
|
|
||
| const r = Math.round(color1.r + (color2.r - color1.r) * t); | ||
| const g = Math.round(color1.g + (color2.g - color1.g) * t); | ||
| const b = Math.round(color1.b + (color2.b - color1.b) * t); | ||
|
|
||
| return rgbToHex(r, g, b); | ||
| } | ||
|
|
||
| function getCurrentColor(value: number): string { | ||
| const v = Math.max(0, Math.min(1, value)); | ||
|
|
||
|
|
||
| const colors: [number, string][] = [ | ||
| [0.0, '#00ff00'], // green | ||
| [0.33, '#ffff00'], // yellow | ||
| [0.66, '#ff8800'], // orange | ||
| [1.0, '#ff0000'], // red | ||
| ]; | ||
|
|
||
| let lower: [number, string] = colors[0]!; | ||
| let upper: [number, string] = colors[colors.length - 1]!; | ||
|
|
||
| for (let i = 0; i < colors.length - 1; i++) { | ||
| const current = colors[i]; | ||
| const next = colors[i + 1]; | ||
| if (current && next && v >= current[0] && v <= next[0]) { | ||
| lower = current; | ||
| upper = next; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const range = upper[0] - lower[0]; | ||
| const t = range === 0 ? 0 : (v - lower[0]) / range; | ||
|
|
||
| const color1 = hexToRgb(lower[1]); | ||
| const color2 = hexToRgb(upper[1]); | ||
|
|
||
| const r = Math.round(color1.r + (color2.r - color1.r) * t); | ||
| const g = Math.round(color1.g + (color2.g - color1.g) * t); | ||
| const b = Math.round(color1.b + (color2.b - color1.b) * t); | ||
|
|
||
| return rgbToHex(r, g, b); | ||
| } | ||
|
|
||
|
|
||
| function getMagmaColor(value: number): string { | ||
| const v = Math.max(0, Math.min(1, value)); | ||
|
|
||
| const colors: [number, string][] = [ | ||
| [0.0, '#fcfdbf'], // light yellow | ||
| [0.25, '#fc8961'], // orange | ||
| [0.5, '#b73779'], // magenta/pink | ||
| [0.75, '#51127c'], // purple | ||
| [1.0, '#000004'], // almost black | ||
| ]; | ||
|
|
||
| let lower: [number, string] = colors[0]!; | ||
| let upper: [number, string] = colors[colors.length - 1]!; | ||
|
|
||
| for (let i = 0; i < colors.length - 1; i++) { | ||
| const current = colors[i]; | ||
| const next = colors[i + 1]; | ||
| if (current && next && v >= current[0] && v <= next[0]) { | ||
| lower = current; | ||
| upper = next; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const range = upper[0] - lower[0]; | ||
| const t = range === 0 ? 0 : (v - lower[0]) / range; | ||
|
|
||
| const color1 = hexToRgb(lower[1]); | ||
| const color2 = hexToRgb(upper[1]); | ||
|
|
||
| const r = Math.round(color1.r + (color2.r - color1.r) * t); | ||
| const g = Math.round(color1.g + (color2.g - color1.g) * t); | ||
| const b = Math.round(color1.b + (color2.b - color1.b) * t); | ||
|
|
||
| return rgbToHex(r, g, b); | ||
| } |
There was a problem hiding this comment.
The three color functions (getViridisColor, getCurrentColor, getMagmaColor) contain nearly identical interpolation logic. The only difference is the color stops array. Consider extracting a single generic interpolateColors(value: number, colors: [number, string][]) function and having each palette function simply call it with its specific color stops. This would eliminate ~60 lines of duplicated code and make it easier to add new palettes in the future.
| <div className="flex flex-col gap-2"> | ||
| <Label htmlFor="color-palette" className="text-sm font-medium"> | ||
| Color Palette | ||
| </Label> | ||
| <Select value={colorPalette} onValueChange={updateColorPalette}> | ||
| <SelectTrigger id="color-palette"> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {Object.entries(ISOCHRONE_PALETTES).map(([key, option]) => ( | ||
| <SelectItem key={key} value={option.value}> | ||
| {option.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> |
There was a problem hiding this comment.
There's already a SelectSetting component (src/components/ui/select-setting.tsx) that wraps a Select with a Label, description tooltip (with a help icon), and consistent styling — matching the pattern used by SliderSetting for other settings in this same panel. Consider using SelectSetting here instead of manually assembling Label, Select, SelectTrigger, etc. This would provide a consistent UI (including a description/help tooltip for the color palette setting) and reduce inline code. The ISOCHRONE_PALETTES entries would need to be mapped to SelectOption format ({ key, text, value }).
| current: { | ||
| label: 'Default', | ||
| value: 'current' as const, |
There was a problem hiding this comment.
The palette name 'current' is ambiguous — it could be confused with "currently selected" rather than "the original/legacy color scheme." Consider renaming it to 'default' or 'classic' to more clearly communicate that this is the original red-green gradient palette. The label in the UI already says "Default", so the internal key should match that meaning.
| if (colorPalette === 'current') { | ||
| fillColor = feature.properties?.fill || '#6200ea'; | ||
| } else { | ||
| const normalizedValue = contourValue / maxRange; |
There was a problem hiding this comment.
Potential division by zero: if maxRange is ever 0, contourValue / maxRange would produce Infinity/NaN, which would then be passed to getIsochroneColor. While the UI slider enforces min={1}, the store doesn't validate maxRange > 0 in updateSettings, so this could occur if state is manipulated from URL params or programmatically. Consider adding a guard such as const normalizedValue = maxRange > 0 ? contourValue / maxRange : 0;.
| const normalizedValue = contourValue / maxRange; | |
| const normalizedValue = maxRange > 0 ? contourValue / maxRange : 0; |
| function getCurrentColor(value: number): string { | ||
| const v = Math.max(0, Math.min(1, value)); | ||
|
|
||
|
|
||
| const colors: [number, string][] = [ | ||
| [0.0, '#00ff00'], // green | ||
| [0.33, '#ffff00'], // yellow | ||
| [0.66, '#ff8800'], // orange | ||
| [1.0, '#ff0000'], // red | ||
| ]; | ||
|
|
||
| let lower: [number, string] = colors[0]!; | ||
| let upper: [number, string] = colors[colors.length - 1]!; | ||
|
|
||
| for (let i = 0; i < colors.length - 1; i++) { | ||
| const current = colors[i]; | ||
| const next = colors[i + 1]; | ||
| if (current && next && v >= current[0] && v <= next[0]) { | ||
| lower = current; | ||
| upper = next; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| const range = upper[0] - lower[0]; | ||
| const t = range === 0 ? 0 : (v - lower[0]) / range; | ||
|
|
||
| const color1 = hexToRgb(lower[1]); | ||
| const color2 = hexToRgb(upper[1]); | ||
|
|
||
| const r = Math.round(color1.r + (color2.r - color1.r) * t); | ||
| const g = Math.round(color1.g + (color2.g - color1.g) * t); | ||
| const b = Math.round(color1.b + (color2.b - color1.b) * t); | ||
|
|
||
| return rgbToHex(r, g, b); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
The getCurrentColor function and its case 'current' path in getIsochroneColor are never actually reached at the call site. In isochrone-polygons.tsx (line 28-29), when colorPalette === 'current', the code uses feature.properties?.fill directly instead of calling getIsochroneColor. This means getCurrentColor is dead code. Consider either:
- Removing the
'current'case shortcut inisochrone-polygons.tsxand always going throughgetIsochroneColor(which would requiregetCurrentColorto use the server-assigned fill color or be redesigned), or - Removing the
getCurrentColorfunction and the'current'case fromgetIsochroneColorsince they're not used.
| function getCurrentColor(value: number): string { | |
| const v = Math.max(0, Math.min(1, value)); | |
| const colors: [number, string][] = [ | |
| [0.0, '#00ff00'], // green | |
| [0.33, '#ffff00'], // yellow | |
| [0.66, '#ff8800'], // orange | |
| [1.0, '#ff0000'], // red | |
| ]; | |
| let lower: [number, string] = colors[0]!; | |
| let upper: [number, string] = colors[colors.length - 1]!; | |
| for (let i = 0; i < colors.length - 1; i++) { | |
| const current = colors[i]; | |
| const next = colors[i + 1]; | |
| if (current && next && v >= current[0] && v <= next[0]) { | |
| lower = current; | |
| upper = next; | |
| break; | |
| } | |
| } | |
| const range = upper[0] - lower[0]; | |
| const t = range === 0 ? 0 : (v - lower[0]) / range; | |
| const color1 = hexToRgb(lower[1]); | |
| const color2 = hexToRgb(upper[1]); | |
| const r = Math.round(color1.r + (color2.r - color1.r) * t); | |
| const g = Math.round(color1.g + (color2.g - color1.g) * t); | |
| const b = Math.round(color1.b + (color2.b - color1.b) * t); | |
| return rgbToHex(r, g, b); | |
| } |
| colorPalette: 'current', | ||
| opacity: 0.4, | ||
| maxRange: 10, |
There was a problem hiding this comment.
Inconsistent indentation: colorPalette, opacity, and maxRange are indented with 4 spaces instead of 2 spaces, which is inconsistent with the rest of the object properties in createMockState (e.g., successful on line 51 uses 2 spaces).
| colorPalette: 'current', | |
| opacity: 0.4, | |
| maxRange: 10, | |
| colorPalette: 'current', | |
| opacity: 0.4, | |
| maxRange: 10, |
| <Label htmlFor="color-palette" className="text-sm font-medium"> | ||
| Color Palette | ||
| </Label> | ||
| <Select value={colorPalette} onValueChange={updateColorPalette}> |
There was a problem hiding this comment.
The Radix Select onValueChange callback provides a plain string, but updateColorPalette expects an IsochronePalette type. While this likely works at runtime since the Select options are restricted to valid palette values, passing updateColorPalette directly is not type-safe. Consider adding a type cast or validation in the handler, e.g., onValueChange={(value) => updateColorPalette(value as IsochronePalette)}.
| <Select value={colorPalette} onValueChange={updateColorPalette}> | |
| <Select | |
| value={colorPalette} | |
| onValueChange={(value) => updateColorPalette(value as IsochronePalette)} | |
| > |
| colorPalette: 'current', | ||
| opacity: 0.4, | ||
| maxRange: 10, |
There was a problem hiding this comment.
The isochrone-polygons tests only exercise the 'current' palette. There's no test verifying that the fillColor property on features is computed correctly when using 'viridis' or 'magma' palettes (i.e., that getIsochroneColor is called with the correct normalized value). Consider adding a test case that uses a non-default colorPalette and verifies the resulting feature colors in the Source data.
| colorPalette: 'current', | ||
| opacity: 0.4, | ||
| updateColorPalette: mockUpdateColorPalette, | ||
| updateOpacity: mockUpdateOpacity, |
There was a problem hiding this comment.
The mocks for updateColorPalette and updateOpacity are added, but there are no corresponding test cases that verify the Color Palette selector and Opacity slider are rendered in the settings panel or that interactions with them invoke the correct store actions. The existing tests verify rendering and interaction for all other settings (maxRange, interval, denoise, generalize). Consider adding similar tests for the new controls to maintain consistent coverage.
| export const ISOCHRONE_PALETTES = { | ||
| current: { | ||
| label: 'Default', | ||
| value: 'current' as const, | ||
| }, | ||
| magma: { | ||
| label: 'Magma (colorblind-friendly)', | ||
| value: 'magma' as const, | ||
| }, | ||
| viridis: { | ||
| label: 'Viridis (colorblind-friendly)', | ||
| value: 'viridis' as const, | ||
| }, | ||
|
|
||
| } as const; |
There was a problem hiding this comment.
The ISOCHRONE_PALETTES object has inconsistent indentation. The current entry uses 5 spaces, magma uses 4 spaces, and viridis uses 2 spaces. These should all use the same indentation level (2 spaces, consistent with the rest of the codebase).
Closes #283
Addresses isochrone color customization issue
🛠️ Fixes Issue
The current isochrone visualization uses a red-green color gradient that is not accessible for colorblind users. Additionally, the colors are too opaque, making it difficult to see the underlying map features.
👨💻 Changes proposed
This PR implements:
1. Colorblind-Friendly Color Palettes
2. Opacity Control
3. Intuitive UI
Technical Implementation
New Files
src/utils/isochrone-colors.ts- Color palette utilities with interpolation functionssrc/utils/isochrone-colors.spec.ts- Comprehensive tests for color interpolation logicModified Files
src/stores/isochrones-store.ts- AddedcolorPaletteandopacitystatesrc/components/isochrones/waypoints.tsx- Added UI controlssrc/components/isochrones/waypoints.spec.tsx- Added missing store mocks for new state fieldssrc/components/map/parts/isochrone-polygons.tsx- Dynamic color applicationsrc/components/map/parts/isochrone-polygons.spec.tsx- Updated tests with contour-based mock dataTesting
📷 Screenshots