Copilot commented on code in PR #36732:
URL: https://github.com/apache/superset/pull/36732#discussion_r2658779489
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,22 +95,127 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorScale = [],
colorScheme,
sliceId,
} = props;
const container = element;
const format = getNumberFormatter(numberFormat);
- const linearColorScale = getSequentialSchemeRegistry()
- .get(linearColorScheme)
- .createLinearScale(d3Extent(data, v => v.metric));
- const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ 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 parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ 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;
+
+ let percentColorScale = null;
+ if (
+ Array.isArray(normalizedScaleWithColors) &&
+ normalizedScaleWithColors.length >= 2
+ ) {
+ const sorted = normalizedScaleWithColors
+ .filter(
+ e => e && typeof e.percent === 'number' && typeof e.color === 'string',
Review Comment:
The check `e && typeof e.percent === 'number' && typeof e.color ===
'string'` on line 138 validates the types but doesn't validate that percent
values are within a reasonable range (0-100). Users could provide percent
values like -50 or 150, which would be sorted and used in the d3 scale domain
but may produce unexpected color mappings. Consider adding validation to ensure
percent values are within the 0-100 range or documenting that values outside
this range are supported.
```suggestion
e =>
e &&
typeof e.percent === 'number' &&
e.percent >= 0 &&
e.percent <= 100 &&
typeof e.color === 'string',
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts:
##########
@@ -69,6 +69,48 @@ const config: ControlPanelConfig = {
},
],
['linear_color_scheme'],
+ [
+ {
+ name: 'customColorScale',
+ config: {
+ type: 'TextAreaControl',
+ label: t('Custom Color Scale (by %)'),
+ description: t(
+ 'Custom JSON configuration that overrides the linear color
scheme color codes and thresholds.<br />Thresholds are defined in percentage,
and color codes accept any valid CSS value.<br />Config must be a valid JSON
excerpt.<br />Copy-paste and adapt following sample configuration to define
your own thresholds and colors :<br />\n' +
+ '[<br />\n' +
+ ' { "percent": 0, "color": "white" },<br />\n' +
+ ' { "percent": 0.01, "color": "#A00000" },<br />\n' +
+ ' { "percent": 20, "color": "#E52B50" },<br />\n' +
+ ' { "percent": 35, "color": "#FFA500" },<br />\n' +
+ ' { "percent": 50, "color": "#FFFF99" },<br />\n' +
+ ' { "percent": 65, "color": "#9ACD32" },<br />\n' +
+ ' { "percent": 80, "color": "#3CB371" },<br />\n' +
+ ' { "percent": 99.99, "color": "#228B22" },<br />\n' +
+ ' { "percent": 100, "color": "black" }<br />\n' +
+ ']',
+ ),
+ default: ``,
+ language: 'json',
+ rows: 12,
+ renderTrigger: true,
+ },
+ },
+ ],
+ [
+ {
+ name: 'pickColor',
+ config: {
+ type: 'ColorPickerControl',
+ label: t('Color selector'),
+ renderTrigger: false,
+ dontRefreshOnChange: false,
+ default: '#000000',
+ description: t(
+ 'Pick a custom color and get its HEX code for use into the
Custom Color Scale configuration.',
Review Comment:
The control labeled 'Color selector' with a default value of '#000000'
doesn't persist its value in the form data or affect the chart behavior. The
description states it's for getting HEX codes, but there's no mechanism to copy
the selected color. Users would need to manually copy the color from the
picker. Consider either adding functionality to copy the selected color to
clipboard or clarifying that users should manually note the color value for use
in the Custom Color Scale.
```suggestion
'Use this color picker to choose a color and note or copy
its HEX code manually for use in the Custom Color Scale configuration. This
control does not directly affect the chart.',
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -41,10 +41,52 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ customColorScale: PropTypes.array,
};
const maps = {};
+function normalizeColorKeyword(color) {
+ if (color == null) return '#000000';
+ const c = String(color).trim();
+
+ // Hex colors (#RGB, #RRGGBB, #RGBA, #RRGGBBAA)
+ if (/^#([0-9a-f]{3}|[0-9a-f]{4}|[0-9a-f]{6}|[0-9a-f]{8})$/i.test(c)) return
c;
+
+ // CSS color functions (rgb, rgba, hsl, hsla) with flexible spacing and alpha
+ const colorFuncRegex =
+
/^(rgb|rgba)\(\s*(\d{1,3}%?\s*,\s*){2}\d{1,3}%?(?:\s*,\s*(\d*\.?\d+))?\s*\)$/i;
+ const colorFuncHslRegex =
+ /^(hsl|hsla)\(\s*\d+\s*,\s*\d+%\s*,\s*\d+%(?:\s*,\s*(\d*\.?\d+))?\s*\)$/i;
+ if (colorFuncRegex.test(c) || colorFuncHslRegex.test(c)) return c;
+
+ // Named CSS colors and system colors
Review Comment:
The color validation regex patterns allow invalid CSS color function
formats. For example, the rgb/rgba regex accepts percentages mixed with
absolute values and doesn't properly validate value ranges (0-255 for rgb, 0-1
for alpha). Colors like 'rgb(999, 500, 300)' or 'rgba(50%, 100, 75, 2)' would
pass validation but are invalid CSS. Consider using a more strict validation or
relying on browser validation by testing the actual computed color value.
```suggestion
// Validate all other CSS color formats (named colors, rgb/rgba, hsl/hsla,
etc.)
// using the browser's CSS parser via a temporary option element.
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts:
##########
@@ -69,6 +69,48 @@ const config: ControlPanelConfig = {
},
],
['linear_color_scheme'],
+ [
+ {
+ name: 'customColorScale',
+ config: {
+ type: 'TextAreaControl',
+ label: t('Custom Color Scale (by %)'),
+ description: t(
+ 'Custom JSON configuration that overrides the linear color
scheme color codes and thresholds.<br />Thresholds are defined in percentage,
and color codes accept any valid CSS value.<br />Config must be a valid JSON
excerpt.<br />Copy-paste and adapt following sample configuration to define
your own thresholds and colors :<br />\n' +
+ '[<br />\n' +
+ ' { "percent": 0, "color": "white" },<br />\n' +
+ ' { "percent": 0.01, "color": "#A00000" },<br />\n' +
+ ' { "percent": 20, "color": "#E52B50" },<br />\n' +
+ ' { "percent": 35, "color": "#FFA500" },<br />\n' +
+ ' { "percent": 50, "color": "#FFFF99" },<br />\n' +
+ ' { "percent": 65, "color": "#9ACD32" },<br />\n' +
+ ' { "percent": 80, "color": "#3CB371" },<br />\n' +
+ ' { "percent": 99.99, "color": "#228B22" },<br />\n' +
+ ' { "percent": 100, "color": "black" }<br />\n' +
+ ']',
+ ),
Review Comment:
The description contains HTML break tags (br) embedded in the translation
string, which is not a best practice for internationalization. HTML in
translatable strings makes it difficult for translators and can cause issues
with different text lengths in various languages. Consider using template
literals with line breaks or handling the formatting outside the translation
function.
```suggestion
description: t(`Custom JSON configuration that overrides the
linear color scheme color codes and thresholds.
Thresholds are defined in percentage, and color codes accept any valid CSS
value.
Config must be a valid JSON excerpt.
Copy-paste and adapt following sample configuration to define your own
thresholds and colors:
[
{ "percent": 0, "color": "white" },
{ "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": "black" }
]`),
```
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,22 +95,127 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorScale = [],
colorScheme,
sliceId,
} = props;
const container = element;
const format = getNumberFormatter(numberFormat);
- const linearColorScale = getSequentialSchemeRegistry()
- .get(linearColorScheme)
- .createLinearScale(d3Extent(data, v => v.metric));
- const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ 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 parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ 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;
+
+ 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)
+ // Remove interpolation to avoid blending between steps - always
return lower boundary
+ .interpolate(function (a, b) {
+ return function () {
+ return a;
+ };
+ });
Review Comment:
The interpolation function implementation creates a step function but
doesn't use the second parameter 'b' or the inner function's parameter. This
creates unnecessary function nesting. Consider simplifying to directly return
the 'a' value, as the interpolation is only called once per domain segment to
create the interpolator.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,22 +95,127 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorScale = [],
colorScheme,
sliceId,
} = props;
const container = element;
const format = getNumberFormatter(numberFormat);
- const linearColorScale = getSequentialSchemeRegistry()
- .get(linearColorScheme)
- .createLinearScale(d3Extent(data, v => v.metric));
- const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ 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 parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ 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;
Review Comment:
The variable 'valueRangeNonZero' is calculated on line 129 but is only used
in one place (line 200). When minValue equals maxValue, the code branches to
use a fixed 50% value instead, making valueRangeNonZero unused in that path.
Consider moving the calculation closer to its usage or directly inline it on
line 200 to improve code clarity and avoid computing a value that might not be
needed.
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,22 +95,127 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorScale = [],
colorScheme,
sliceId,
} = props;
const container = element;
const format = getNumberFormatter(numberFormat);
- const linearColorScale = getSequentialSchemeRegistry()
- .get(linearColorScheme)
- .createLinearScale(d3Extent(data, v => v.metric));
- const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ 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 parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ 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;
+
+ 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)
+ // Remove interpolation to avoid blending between steps - always
return lower boundary
+ .interpolate(function (a, b) {
+ return function () {
+ return a;
+ };
+ });
+ }
+ }
+
+ 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;
+ }
+ }
+
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;
+ }
+
+ if (percentColorScale) {
+ 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
+ }
+ }
+ } else if (linearPaletteScale) {
+ try {
+ colorMap[iso] = linearPaletteScale(value);
+ return;
+ } catch {
+ // continue regardless of error
+ }
Review Comment:
Empty catch blocks (lines 195-197, 205-207, 213-215) silently swallow errors
without any logging or fallback indication. While the code continues to the
next color assignment method, users won't know why their custom color scale
failed. Consider adding console.warn statements in these catch blocks to help
users debug issues with their custom color configurations.
--
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]
