feat: add custom colors and thresholds for CountryMap plugin #36103
Conversation
Code Review Agent Run #3fbf4fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/ReactCountryMap.jsx | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
There was a problem hiding this comment.
Pull Request Overview
This PR adds JSON-based customization capabilities to the CountryMap plugin, allowing users to override the default linear color scheme with custom colors and percentage-based thresholds. The feature is configurable through a new "Custom Color Scale" setting in the Chart Options panel.
Key Changes
- Added a new
customColorScaleTextAreaControl that accepts JSON configuration for color thresholds defined by percentages - Implemented color priority logic: conditional rules > custom percentage scale > linear palette > gradient fallback
- Added helper functions for color normalization, safe number parsing, and RGBA to hex conversion
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| transformProps.js | Parses JSON configuration from form data for custom color rules and scales |
| controlPanel.ts | Adds new TextAreaControl for custom color scale configuration with JSON syntax |
| ReactCountryMap.jsx | Minor formatting adjustments (whitespace) |
| CountryMap.js | Core implementation of custom color scaling logic with multiple fallback strategies |
| function normalizeColorKeyword(color) { | ||
| if (!color && color !== '') return '#000000'; | ||
| const c = String(color).trim().toLowerCase(); | ||
| if (/^#([0-9a-f]{3}|[0-9a-f]{6})$/i.test(c)) return c; | ||
| if (/^[a-z]+$/.test(c)) return c; | ||
|
|
||
| return '#000000'; | ||
| } |
There was a problem hiding this comment.
The normalizeColorKeyword function doesn't validate or normalize other valid CSS color formats like rgb(), rgba(), hsl(), hsla(), or named colors beyond simple keywords. The function name suggests it normalizes color keywords, but the current implementation will return '#000000' for valid CSS colors that don't match the two regex patterns, which could lead to unexpected behavior when users provide valid CSS colors in their custom configuration.
Consider either:
- Renaming the function to reflect its limited scope (e.g.,
validateBasicColor) - Expanding validation to handle more CSS color formats
- Adding documentation that only hex colors and simple lowercase color names are supported
| { | ||
| name: 'customColorScale', | ||
| config: { | ||
| type: 'TextAreaControl', | ||
| label: t('Custom Color Scale (by %)'), | ||
| description: t( | ||
| "Custom configuration that overrides the linear color scheme color codes and thresholds. Thresholds are defined in percentage, and color codes accept any valid CSS value. Copy-paste and edit following sample configuration to define your own thresholds and colors :\n\n" + | ||
| "[\n" + | ||
| " { \"percent\": 0, \"color\": \"#600000\" },\n" + | ||
| " { \"percent\": 0.01, \"color\": \"#A00000\" },\n" + | ||
| " { \"percent\": 20, \"color\": \"#E52B50\" },\n" + | ||
| " { \"percent\": 35, \"color\": \"#FFA500\" },\n" + | ||
| " { \"percent\": 50, \"color\": \"#FFFF99\" },\n" + | ||
| " { \"percent\": 65, \"color\": \"#9ACD32\" },\n" + | ||
| " { \"percent\": 80, \"color\": \"#3CB371\" },\n" + | ||
| " { \"percent\": 99.99, \"color\": \"#228B22\" },\n" + | ||
| " { \"percent\": 100, \"color\": \"#006400\" }\n" + | ||
| "]" | ||
| ), | ||
| default: `[ | ||
| { "percent": 0, "color": "#600000" }, | ||
| { "percent": 0.01, "color": "#A00000" }, | ||
| { "percent": 20, "color": "#E52B50" }, | ||
| { "percent": 35, "color": "#FFA500" }, | ||
| { "percent": 50, "color": "#FFFF99" }, | ||
| { "percent": 65, "color": "#9ACD32" }, | ||
| { "percent": 80, "color": "#3CB371" }, | ||
| { "percent": 99.99, "color": "#228B22" }, | ||
| { "percent": 100, "color": "#006400" } | ||
| ]`, | ||
| language: 'json', | ||
| rows: 10, | ||
| renderTrigger: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
[nitpick] Missing input validation for the JSON in the TextAreaControl. Users could enter malformed JSON or data that doesn't match the expected schema (objects without 'percent' or 'color' fields, incorrect types, etc.). While there is try-catch in transformProps.js, invalid data that successfully parses as JSON will silently fail or produce unexpected results.
Consider adding validation logic or using a more structured control (like a JSON editor with schema validation) to provide immediate feedback to users when they enter invalid configurations.
| const percentNormalized = ((value - minValue) / valueRangeNonZero) * 100; | ||
| const p = Math.max(0, Math.min(100, percentNormalized)); | ||
| try { | ||
| colorMap[iso] = percentColorScale(p); | ||
| return; | ||
| } catch { | ||
| // continue regardless of error |
There was a problem hiding this comment.
The percentage calculation on line 266 uses valueRangeNonZero to avoid division by zero, but the logic for handling this edge case could lead to incorrect results. When minValue === maxValue, all values will be normalized to 0% (since (value - minValue) / 1 * 100 = 0), but they should probably all map to 100% or 50% to indicate they're at the singular data point.
Consider handling this case explicitly:
if (valueRange === 0) {
colorMap[iso] = percentColorScale(50); // or 100, depending on desired behavior
return;
}| const percentNormalized = ((value - minValue) / valueRangeNonZero) * 100; | |
| const p = Math.max(0, Math.min(100, percentNormalized)); | |
| try { | |
| colorMap[iso] = percentColorScale(p); | |
| return; | |
| } catch { | |
| // continue regardless of error | |
| if (minValue === maxValue) { | |
| // All values are the same; map to central color (e.g., 50%) | |
| try { | |
| colorMap[iso] = percentColorScale(50); | |
| return; | |
| } catch { | |
| // continue regardless of error | |
| } | |
| } else { | |
| const percentNormalized = ((value - minValue) / valueRangeNonZero) * 100; | |
| const p = Math.max(0, Math.min(100, percentNormalized)); | |
| try { | |
| colorMap[iso] = percentColorScale(p); | |
| return; | |
| } catch { | |
| // continue regardless of error | |
| } |
| customColorRules: PropTypes.array, | ||
| minColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]), | ||
| maxColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]), | ||
| customColorScale: PropTypes.oneOfType([PropTypes.array, PropTypes.string]), |
There was a problem hiding this comment.
The PropType for customColorScale is defined as PropTypes.oneOfType([PropTypes.array, PropTypes.string]) but based on the parsing logic in transformProps.js, it should always be an array by the time it reaches this component. The string type appears unnecessary and creates confusion about the expected data type.
Consider changing to PropTypes.array only, or add a comment explaining why it can be a string.
| customColorScale: PropTypes.oneOfType([PropTypes.array, PropTypes.string]), | |
| customColorScale: PropTypes.array, |
| const getColorFromRules = value => { | ||
| if (!Array.isArray(customColorRules) || !Number.isFinite(value)) | ||
| return null; | ||
| for (const rule of customColorRules) { | ||
| if ( | ||
| rule && | ||
| typeof rule.color === 'string' && | ||
| (('min' in rule && 'max' in rule) || 'value' in rule) | ||
| ) { | ||
| if ('value' in rule && Number(rule.value) === value) { | ||
| // 🆕 normalize possible keyword in conditional rules as well | ||
| return normalizeColorKeyword(rule.color); | ||
| } | ||
| if ('min' in rule && 'max' in rule) { | ||
| const minR = safeNumber(rule.min); | ||
| const maxR = safeNumber(rule.max); | ||
| if ( | ||
| Number.isFinite(minR) && | ||
| Number.isFinite(maxR) && | ||
| value >= minR && | ||
| value <= maxR | ||
| ) { | ||
| return normalizeColorKeyword(rule.color); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The color mapping logic iterates through customColorRules array for every data point on line 156. If there are many rules and many data points, this could lead to O(n*m) complexity. Consider optimizing by:
- Pre-processing rules into a more efficient data structure (e.g., sorted array for binary search, or hash map for exact value matches)
- Early termination when a rule matches (which is already done with
return)
However, this is likely acceptable for typical use cases with small numbers of rules.
| const getColorFromRules = value => { | |
| if (!Array.isArray(customColorRules) || !Number.isFinite(value)) | |
| return null; | |
| for (const rule of customColorRules) { | |
| if ( | |
| rule && | |
| typeof rule.color === 'string' && | |
| (('min' in rule && 'max' in rule) || 'value' in rule) | |
| ) { | |
| if ('value' in rule && Number(rule.value) === value) { | |
| // 🆕 normalize possible keyword in conditional rules as well | |
| return normalizeColorKeyword(rule.color); | |
| } | |
| if ('min' in rule && 'max' in rule) { | |
| const minR = safeNumber(rule.min); | |
| const maxR = safeNumber(rule.max); | |
| if ( | |
| Number.isFinite(minR) && | |
| Number.isFinite(maxR) && | |
| value >= minR && | |
| value <= maxR | |
| ) { | |
| return normalizeColorKeyword(rule.color); | |
| } | |
| } | |
| } | |
| } | |
| // Preprocess customColorRules for efficient lookup | |
| let valueRuleMap = {}; | |
| let rangeRules = []; | |
| if (Array.isArray(customColorRules)) { | |
| for (const rule of customColorRules) { | |
| if ( | |
| rule && | |
| typeof rule.color === 'string' && | |
| (('min' in rule && 'max' in rule) || 'value' in rule) | |
| ) { | |
| if ('value' in rule && Number.isFinite(Number(rule.value))) { | |
| valueRuleMap[Number(rule.value)] = normalizeColorKeyword(rule.color); | |
| } else if ('min' in rule && 'max' in rule) { | |
| const minR = safeNumber(rule.min); | |
| const maxR = safeNumber(rule.max); | |
| if (Number.isFinite(minR) && Number.isFinite(maxR)) { | |
| rangeRules.push({ | |
| min: minR, | |
| max: maxR, | |
| color: normalizeColorKeyword(rule.color), | |
| }); | |
| } | |
| } | |
| } | |
| } | |
| // Sort rangeRules by min for possible future optimizations | |
| rangeRules.sort((a, b) => a.min - b.min); | |
| } | |
| const getColorFromRules = value => { | |
| if (!Number.isFinite(value)) return null; | |
| // Check for exact value match first | |
| if (Object.prototype.hasOwnProperty.call(valueRuleMap, value)) { | |
| return valueRuleMap[value]; | |
| } | |
| // Check range rules | |
| for (const rule of rangeRules) { | |
| if (value >= rule.min && value <= rule.max) { | |
| return rule.color; | |
| } | |
| } |
| try { | ||
| parsedColorScale = customColorScale ? JSON.parse(customColorScale) : []; | ||
| } catch (error) { | ||
| console.warn('Invalid JSON in customColorScale:', error); |
There was a problem hiding this comment.
The console.warn messages for JSON parsing errors will only show generic error messages. This makes it difficult for users to debug their JSON configuration issues. Consider providing more specific error messages that include what went wrong and where.
Example:
console.warn('Invalid JSON in customColorScale. Please check your configuration syntax:', error.message);| console.warn('Invalid JSON in customColorScale:', error); | |
| console.warn( | |
| 'Invalid JSON in customColorScale. Please check your configuration syntax. Error:', | |
| error && error.message ? error.message : error, | |
| 'Input:', | |
| customColorScale, | |
| ); |
| } | ||
|
|
||
| /** ------------------------- | ||
| * 3) Linear palette from registry (SI défini) |
There was a problem hiding this comment.
Typo in French comment: "SI défini" should be "if defined" in English to maintain consistency with the rest of the codebase.
| * 3) Linear palette from registry (SI défini) | |
| * 3) Linear palette from registry (if defined) |
| if (!color && color !== '') return '#000000'; | ||
| const c = String(color).trim().toLowerCase(); | ||
| if (/^#([0-9a-f]{3}|[0-9a-f]{6})$/i.test(c)) return c; | ||
| if (/^[a-z]+$/.test(c)) return c; |
There was a problem hiding this comment.
The regex pattern /^[a-z]+$/ only matches lowercase color keywords, but CSS color keywords are case-insensitive and could be provided in uppercase or mixed case (e.g., "Red", "BLUE"). This will cause valid CSS color names to fall through to the default '#000000'.
Consider making the pattern case-insensitive: /^[a-z]+$/i or converting the color to lowercase before testing: if (/^[a-z]+$/.test(c.toLowerCase()))
| console.warn('Invalid JSON in customColorRules:', error); | ||
| } | ||
|
|
||
| try { | ||
| parsedColorScale = customColorScale ? JSON.parse(customColorScale) : []; | ||
| } catch (error) { | ||
| console.warn('Invalid JSON in customColorScale:', error); |
There was a problem hiding this comment.
The console.warn messages for JSON parsing errors will only show generic error messages. This makes it difficult for users to debug their JSON configuration issues. Consider providing more specific error messages that include what went wrong and where.
Example:
console.warn('Invalid JSON in customColorRules. Please check your configuration syntax:', error.message);| console.warn('Invalid JSON in customColorRules:', error); | |
| } | |
| try { | |
| parsedColorScale = customColorScale ? JSON.parse(customColorScale) : []; | |
| } catch (error) { | |
| console.warn('Invalid JSON in customColorScale:', error); | |
| console.warn( | |
| 'Invalid JSON in customColorRules. Please check your configuration syntax:', | |
| error && error.message ? error.message : error | |
| ); | |
| } | |
| try { | |
| parsedColorScale = customColorScale ? JSON.parse(customColorScale) : []; | |
| } catch (error) { | |
| console.warn( | |
| 'Invalid JSON in customColorScale. Please check your configuration syntax:', | |
| error && error.message ? error.message : error | |
| ); |
| const linearColorScale = getSequentialSchemeRegistry() | ||
| .get(linearColorScheme) | ||
| .createLinearScale(d3Extent(data, v => v.metric)); |
There was a problem hiding this comment.
The variable linearColorScale is created on line 124-126 but is never used in the updated code. The new implementation uses linearPaletteScale instead. This unused variable should be removed to avoid confusion and maintain code cleanliness.
| const linearColorScale = getSequentialSchemeRegistry() | |
| .get(linearColorScheme) | |
| .createLinearScale(d3Extent(data, v => v.metric)); |
1 participant
SUMMARY
Add a JSON-based customisation block that allows to override linearColorScheme colors and thresholds.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
A new setting is available is "Chart Options" panel
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION