Copilot commented on code in PR #36103:
URL: https://github.com/apache/superset/pull/36103#discussion_r2535114631
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,24 +98,208 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorRules = [],
+ customColorScale = [],
+ minColor,
+ maxColor,
colorScheme,
sliceId,
} = props;
+ const minColorHexRaw = rgbaToHex(minColor) || '#f7fbff';
+ const maxColorHexRaw = rgbaToHex(maxColor) || '#08306b';
+ const minColorHex = normalizeColorKeyword(minColorHexRaw);
+ const maxColorHex = normalizeColorKeyword(maxColorHexRaw);
+
const container = element;
const format = getNumberFormatter(numberFormat);
+ const normalizedScale = normalizeScale(customColorScale);
+ const normalizedScaleWithColors = Array.isArray(normalizedScale)
+ ? normalizedScale.map(e => {
+ if (!e || typeof e !== 'object') return e;
+ return { ...e, color: normalizeColorKeyword(e.color) };
+ })
+ : [];
+
const linearColorScale = getSequentialSchemeRegistry()
.get(linearColorScheme)
.createLinearScale(d3Extent(data, v => v.metric));
const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ // Parse metrics to numbers safely
+ const parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ // numeric values only
+ const numericValues = parsedData
+ .map(r => r.metric)
+ .filter(v => Number.isFinite(v));
+
+ let minValue = 0;
+ let maxValue = 1;
+ if (numericValues.length > 0) {
+ const extent = d3Extent(numericValues);
+ minValue = extent[0];
+ maxValue = extent[1];
+ }
+
+ const valueRange = maxValue - minValue;
+ const valueRangeNonZero = valueRange === 0 ? 1 : valueRange;
+
+ /** -------------------------
+ * 1) Custom conditional 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);
+ }
+ }
+ }
+ }
+ return null;
+ };
+
+ /** -------------------------
+ * 2) Custom color scale (by %)
+ * ------------------------- */
+ let percentColorScale = null;
+ if (
+ Array.isArray(normalizedScaleWithColors) &&
+ normalizedScaleWithColors.length >= 2
+ ) {
+ const sorted = normalizedScaleWithColors
+ .filter(
+ e => e && typeof e.percent === 'number' && typeof e.color === 'string',
+ )
+ .slice()
+ .sort((a, b) => a.percent - b.percent);
+
+ if (sorted.length >= 2) {
+ const domainPerc = sorted.map(e => e.percent);
+ const rangeColors = sorted.map(e => e.color);
+ percentColorScale = d3.scale
+ .linear()
+ .domain(domainPerc)
+ .range(rangeColors)
+ .clamp(true)
+ .interpolate(d3.interpolateRgb);
+ }
+ }
+
+ /** -------------------------
+ * 3) Linear palette from registry (SI défini)
+ * ------------------------- */
+ let linearPaletteScale = null;
+ if (linearColorScheme) {
+ try {
+ const seq = getSequentialSchemeRegistry().get(linearColorScheme);
+ if (seq && typeof seq.createLinearScale === 'function') {
+ linearPaletteScale = seq.createLinearScale([minValue, maxValue]);
+ } else if (seq && Array.isArray(seq.colors) && seq.colors.length >= 2) {
+ linearPaletteScale = d3.scale
+ .linear()
+ .domain([minValue, maxValue])
+ .range([seq.colors[0], seq.colors[seq.colors.length - 1]])
+ .interpolate(d3.interpolateRgb);
+ }
+ } catch {
+ linearPaletteScale = null;
+ }
+ }
+
+ /** -------------------------
+ * 4) Gradient fallback (minColor → maxColor) avec HEX
+ * ------------------------- */
+ let gradientColorScale;
+ if (minValue === maxValue) {
+ gradientColorScale = () => minColorHex || maxColorHex || '#ddd';
+ } else {
+ gradientColorScale = d3.scale
+ .linear()
+ .domain([minValue, maxValue])
+ .range([minColorHex, maxColorHex])
+ .interpolate(d3.interpolateRgb);
+ }
+
+ /** -------------------------
+ * Build final color (priority)
+ * rules > customScale > linearPalette > gradient
+ * ------------------------- */
const colorMap = {};
- data.forEach(d => {
- colorMap[d.country_id] = colorScheme
- ? colorScale(d.country_id, sliceId)
- : linearColorScale(d.metric);
+ parsedData.forEach(r => {
+ const iso = r.country_id;
+ const value = r.metric;
+ if (!iso) return;
+ if (!Number.isFinite(value)) {
+ colorMap[iso] = 'none';
+ return;
+ }
+
+ const ruleColor = getColorFromRules(value);
+ if (ruleColor) {
+ colorMap[iso] = ruleColor;
+ return;
+ }
+
+ if (percentColorScale) {
+ 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
Review 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:
```javascript
if (valueRange === 0) {
colorMap[iso] = percentColorScale(50); // or 100, depending on desired
behavior
return;
}
```
```suggestion
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
}
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -27,6 +27,15 @@ import {
} from '@superset-ui/core';
import countries, { countryOptions } from './countries';
+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';
+}
Review 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:
1. Renaming the function to reflect its limited scope (e.g.,
`validateBasicColor`)
2. Expanding validation to handle more CSS color formats
3. Adding documentation that only hex colors and simple lowercase color
names are supported
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -27,6 +27,15 @@ import {
} from '@superset-ui/core';
import countries, { countryOptions } from './countries';
+function normalizeColorKeyword(color) {
+ if (!color && color !== '') return '#000000';
Review Comment:
The `normalizeColorKeyword` function has a logical error in the first
condition. The check `!color && color !== ''` will never be true for an empty
string because empty string is falsy. This means empty strings will not be
normalized to '#000000' as intended.
Consider changing to: `if (!color || color === '') return '#000000';` or
simply `if (!color) return '#000000';` since empty string is falsy.
```suggestion
if (!color) return '#000000';
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts:
##########
@@ -69,6 +69,43 @@ const config: ControlPanelConfig = {
},
],
['linear_color_scheme'],
+ [
+ {
+ 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,
+ },
+ },
Review 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.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -41,10 +50,46 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ sliceId: PropTypes.number,
+ customColorRules: PropTypes.array,
+ minColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
+ maxColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
+ customColorScale: PropTypes.oneOfType([PropTypes.array, PropTypes.string]),
Review 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.
```suggestion
customColorScale: PropTypes.array,
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,24 +98,208 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorRules = [],
+ customColorScale = [],
+ minColor,
+ maxColor,
colorScheme,
sliceId,
} = props;
+ const minColorHexRaw = rgbaToHex(minColor) || '#f7fbff';
+ const maxColorHexRaw = rgbaToHex(maxColor) || '#08306b';
+ const minColorHex = normalizeColorKeyword(minColorHexRaw);
+ const maxColorHex = normalizeColorKeyword(maxColorHexRaw);
+
const container = element;
const format = getNumberFormatter(numberFormat);
+ const normalizedScale = normalizeScale(customColorScale);
+ const normalizedScaleWithColors = Array.isArray(normalizedScale)
+ ? normalizedScale.map(e => {
+ if (!e || typeof e !== 'object') return e;
+ return { ...e, color: normalizeColorKeyword(e.color) };
+ })
+ : [];
+
const linearColorScale = getSequentialSchemeRegistry()
.get(linearColorScheme)
.createLinearScale(d3Extent(data, v => v.metric));
const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ // Parse metrics to numbers safely
+ const parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ // numeric values only
+ const numericValues = parsedData
+ .map(r => r.metric)
+ .filter(v => Number.isFinite(v));
+
+ let minValue = 0;
+ let maxValue = 1;
+ if (numericValues.length > 0) {
+ const extent = d3Extent(numericValues);
+ minValue = extent[0];
+ maxValue = extent[1];
+ }
+
+ const valueRange = maxValue - minValue;
+ const valueRangeNonZero = valueRange === 0 ? 1 : valueRange;
+
+ /** -------------------------
+ * 1) Custom conditional 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);
+ }
+ }
+ }
+ }
Review 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:
1. Pre-processing rules into a more efficient data structure (e.g., sorted
array for binary search, or hash map for exact value matches)
2. 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.
```suggestion
// 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;
}
}
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,24 +98,208 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorRules = [],
+ customColorScale = [],
+ minColor,
+ maxColor,
colorScheme,
sliceId,
} = props;
+ const minColorHexRaw = rgbaToHex(minColor) || '#f7fbff';
+ const maxColorHexRaw = rgbaToHex(maxColor) || '#08306b';
+ const minColorHex = normalizeColorKeyword(minColorHexRaw);
+ const maxColorHex = normalizeColorKeyword(maxColorHexRaw);
+
const container = element;
const format = getNumberFormatter(numberFormat);
+ const normalizedScale = normalizeScale(customColorScale);
+ const normalizedScaleWithColors = Array.isArray(normalizedScale)
+ ? normalizedScale.map(e => {
+ if (!e || typeof e !== 'object') return e;
+ return { ...e, color: normalizeColorKeyword(e.color) };
+ })
+ : [];
+
const linearColorScale = getSequentialSchemeRegistry()
.get(linearColorScheme)
.createLinearScale(d3Extent(data, v => v.metric));
const colorScale = CategoricalColorNamespace.getScale(colorScheme);
Review Comment:
The variable `colorScale` is created on line 127 but is replaced by
`fallbackCategorical` on line 291, making this initial assignment unused.
Consider removing line 127 to improve code clarity.
```suggestion
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js:
##########
@@ -24,8 +24,25 @@ export default function transformProps(chartProps) {
selectCountry,
colorScheme,
sliceId,
+ customColorRules,
Review Comment:
The variable name `customColorRules` is extracted from `formData` but this
feature is not actually implemented or used anywhere in the visible changes.
The parsing logic is added in `transformProps.js` and the prop is defined in
PropTypes, but there's no corresponding control in `controlPanel.ts` and no
documentation on how to use it. This creates an incomplete API surface.
Either add the corresponding control and documentation for this feature, or
remove it if it's not part of this PR's scope.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js:
##########
@@ -24,8 +24,25 @@ export default function transformProps(chartProps) {
selectCountry,
colorScheme,
sliceId,
+ customColorRules,
+ customColorScale,
} = formData;
+ let parsedColorRules = [];
+ let parsedColorScale = [];
+
+ try {
+ parsedColorRules = customColorRules ? JSON.parse(customColorRules) : [];
+ } catch (error) {
+ console.warn('Invalid JSON in customColorRules:', error);
+ }
+
+ try {
+ parsedColorScale = customColorScale ? JSON.parse(customColorScale) : [];
+ } catch (error) {
+ console.warn('Invalid JSON in customColorScale:', error);
Review 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:
```javascript
console.warn('Invalid JSON in customColorScale. Please check your
configuration syntax:', error.message);
```
```suggestion
console.warn(
'Invalid JSON in customColorScale. Please check your configuration
syntax. Error:',
error && error.message ? error.message : error,
'Input:',
customColorScale,
);
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,24 +98,208 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorRules = [],
+ customColorScale = [],
+ minColor,
+ maxColor,
colorScheme,
sliceId,
} = props;
+ const minColorHexRaw = rgbaToHex(minColor) || '#f7fbff';
+ const maxColorHexRaw = rgbaToHex(maxColor) || '#08306b';
+ const minColorHex = normalizeColorKeyword(minColorHexRaw);
+ const maxColorHex = normalizeColorKeyword(maxColorHexRaw);
+
const container = element;
const format = getNumberFormatter(numberFormat);
+ const normalizedScale = normalizeScale(customColorScale);
+ const normalizedScaleWithColors = Array.isArray(normalizedScale)
+ ? normalizedScale.map(e => {
+ if (!e || typeof e !== 'object') return e;
+ return { ...e, color: normalizeColorKeyword(e.color) };
+ })
+ : [];
+
const linearColorScale = getSequentialSchemeRegistry()
.get(linearColorScheme)
.createLinearScale(d3Extent(data, v => v.metric));
const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ // Parse metrics to numbers safely
+ const parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ // numeric values only
+ const numericValues = parsedData
+ .map(r => r.metric)
+ .filter(v => Number.isFinite(v));
+
+ let minValue = 0;
+ let maxValue = 1;
+ if (numericValues.length > 0) {
+ const extent = d3Extent(numericValues);
+ minValue = extent[0];
+ maxValue = extent[1];
+ }
+
+ const valueRange = maxValue - minValue;
+ const valueRangeNonZero = valueRange === 0 ? 1 : valueRange;
+
+ /** -------------------------
+ * 1) Custom conditional 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);
+ }
+ }
+ }
+ }
+ return null;
+ };
+
+ /** -------------------------
+ * 2) Custom color scale (by %)
+ * ------------------------- */
+ let percentColorScale = null;
+ if (
+ Array.isArray(normalizedScaleWithColors) &&
+ normalizedScaleWithColors.length >= 2
+ ) {
+ const sorted = normalizedScaleWithColors
+ .filter(
+ e => e && typeof e.percent === 'number' && typeof e.color === 'string',
+ )
+ .slice()
+ .sort((a, b) => a.percent - b.percent);
+
+ if (sorted.length >= 2) {
+ const domainPerc = sorted.map(e => e.percent);
+ const rangeColors = sorted.map(e => e.color);
+ percentColorScale = d3.scale
+ .linear()
+ .domain(domainPerc)
+ .range(rangeColors)
+ .clamp(true)
+ .interpolate(d3.interpolateRgb);
+ }
+ }
+
+ /** -------------------------
+ * 3) Linear palette from registry (SI défini)
Review Comment:
Typo in French comment: "SI défini" should be "if defined" in English to
maintain consistency with the rest of the codebase.
```suggestion
* 3) Linear palette from registry (if defined)
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -27,6 +27,15 @@ import {
} from '@superset-ui/core';
import countries, { countryOptions } from './countries';
+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;
Review 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()))`
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,24 +98,208 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorRules = [],
+ customColorScale = [],
+ minColor,
+ maxColor,
colorScheme,
sliceId,
} = props;
+ const minColorHexRaw = rgbaToHex(minColor) || '#f7fbff';
+ const maxColorHexRaw = rgbaToHex(maxColor) || '#08306b';
+ const minColorHex = normalizeColorKeyword(minColorHexRaw);
+ const maxColorHex = normalizeColorKeyword(maxColorHexRaw);
+
const container = element;
const format = getNumberFormatter(numberFormat);
+ const normalizedScale = normalizeScale(customColorScale);
+ const normalizedScaleWithColors = Array.isArray(normalizedScale)
+ ? normalizedScale.map(e => {
+ if (!e || typeof e !== 'object') return e;
+ return { ...e, color: normalizeColorKeyword(e.color) };
+ })
+ : [];
+
const linearColorScale = getSequentialSchemeRegistry()
.get(linearColorScheme)
.createLinearScale(d3Extent(data, v => v.metric));
Review 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.
```suggestion
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -41,10 +50,46 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ sliceId: PropTypes.number,
+ customColorRules: PropTypes.array,
+ minColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
+ maxColor: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
+ customColorScale: PropTypes.oneOfType([PropTypes.array, PropTypes.string]),
};
const maps = {};
+function safeNumber(v) {
+ if (v === null || v === undefined || v === '') return NaN;
+ const n = Number(v);
+ return Number.isFinite(n) ? n : NaN;
+}
+
+function rgbaToHex(rgba) {
+ if (typeof rgba === 'string') return rgba;
+ if (Array.isArray(rgba)) return rgbaToHex(rgba[0]);
+ if (!rgba || typeof rgba !== 'object') return null;
+ const { r, g, b } = rgba;
+ if (r === undefined || g === undefined || b === undefined) return null;
+ const toHex = n => {
+ const hex = Math.round(n).toString(16);
+ return hex.length === 1 ? '0' + hex : hex;
+ };
+ return `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+}
+
+function normalizeScale(scale) {
+ if (Array.isArray(scale)) return scale;
+ if (typeof scale === 'string') {
+ try {
+ return JSON.parse(scale);
+ } catch {
+ return [];
+ }
+ }
+ return [];
+}
Review Comment:
The `normalizeScale` function has duplicate JSON parsing logic that already
exists in `transformProps.js`. The `customColorScale` prop should already be an
array by the time it reaches `CountryMap.js`. This redundant parsing suggests
either:
1. The prop type is incorrectly defined (should be array-only, not
`PropTypes.oneOfType([PropTypes.array, PropTypes.string])`)
2. There's a misunderstanding of the data flow
Consider removing this duplicate parsing logic and relying on the parsing
done in `transformProps.js`, or clarify why both are needed.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/ReactCountryMap.jsx:
##########
@@ -23,8 +23,11 @@ import Component from './CountryMap';
const ReactComponent = reactify(Component);
const CountryMap = ({ className, ...otherProps }) => (
- <div className={className}>
- <ReactComponent {...otherProps} />
+ <div className={className} >
+ <ReactComponent
+ {...otherProps}
+ />
+
Review Comment:
[nitpick] Trailing whitespace on line 30. This should be removed to maintain
code cleanliness.
```suggestion
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js:
##########
@@ -24,8 +24,25 @@ export default function transformProps(chartProps) {
selectCountry,
colorScheme,
sliceId,
+ customColorRules,
+ customColorScale,
} = formData;
+ let parsedColorRules = [];
+ let parsedColorScale = [];
+
+ try {
+ parsedColorRules = customColorRules ? JSON.parse(customColorRules) : [];
+ } catch (error) {
+ console.warn('Invalid JSON in customColorRules:', error);
+ }
+
+ try {
+ parsedColorScale = customColorScale ? JSON.parse(customColorScale) : [];
+ } catch (error) {
+ console.warn('Invalid JSON in customColorScale:', error);
Review 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:
```javascript
console.warn('Invalid JSON in customColorRules. Please check your
configuration syntax:', error.message);
```
```suggestion
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
);
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts:
##########
@@ -69,6 +69,43 @@ const config: ControlPanelConfig = {
},
],
['linear_color_scheme'],
+ [
+ {
+ 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" +
+ "]"
+ ),
Review Comment:
[nitpick] The description string concatenation on lines 79-91 is hard to
read and maintain. Consider using a template literal instead:
```javascript
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 :
[
{ "percent": 0, "color": "#600000" },
{ "percent": 0.01, "color": "#A00000" },
...
]`
)
```
```suggestion
`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 :
[
{ "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" }
]`
),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
