Copilot commented on code in PR #42941:
URL: https://github.com/apache/superset/pull/42941#discussion_r3893378686
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/types.ts:
##########
@@ -51,6 +51,7 @@ export type ConditionalFormattingControlProps =
ControlComponentProps<
description: string;
extraColorChoices?: { label: string; colors: string[] }[];
allColumns?: ColumnOption[];
+ columnMetricFlag?: boolean;
Review Comment:
`columnMetricFlag` is hard to interpret from the name (it reads like a
boolean about columns vs metrics, but it’s used to switch the control into a
metric-only UI mode). Consider renaming to something more explicit like
`metricOnly`, `isMetricOnlyMode`, or `hideFormattingTargets`, or add a short
doc comment explaining what behavior the flag enables/disable.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -262,8 +264,10 @@ export const FormattingPopoverContent = ({
config?.column || columns[0]?.value,
);
const visibleAllColumns = useMemo(
- () => !!(allColumns && Array.isArray(allColumns) && allColumns.length),
- [allColumns],
+ () =>
+ !columnMetricFlag &&
+ !!(allColumns && Array.isArray(allColumns) && allColumns.length),
+ [allColumns, columnMetricFlag],
);
Review Comment:
When `columnMetricFlag` is true the UI hides the
`columnFormatting`/`objectFormatting` fields, but the form can still carry
previously-initialized values from `initialValues={config}` (AntD Form
preserves unmounted fields by default). This can cause hidden/stale
formatting-target values to be submitted even though the user cannot see/edit
them in metric-only mode. Recommended fix: when `columnMetricFlag` is true,
explicitly clear/omit those fields on submit (e.g., wrap `onFinish` to strip
them), and/or set the hidden Form.Items to `preserve={false}` / reset those
fields in an effect when toggling into metric-only mode.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/types.ts:
##########
@@ -61,6 +62,7 @@ export type FormattingPopoverProps = PopoverProps & {
children: ReactNode;
extraColorChoices?: { label: string; colors: string[] }[];
allColumns?: ColumnOption[];
+ columnMetricFlag?: boolean;
Review Comment:
`columnMetricFlag` is hard to interpret from the name (it reads like a
boolean about columns vs metrics, but it’s used to switch the control into a
metric-only UI mode). Consider renaming to something more explicit like
`metricOnly`, `isMetricOnlyMode`, or `hideFormattingTargets`, or add a short
doc comment explaining what behavior the flag enables/disable.
##########
superset-frontend/plugins/plugin-chart-country-map/test/CountryMap.test.tsx:
##########
@@ -246,3 +246,129 @@ describe('CountryMap (legacy d3)', () => {
});
});
});
+
+describe('CountryMap conditional formatting', () => {
+ beforeEach(() => {
+ jest.clearAllMocks();
+ });
+
+ test('applies conditional formatting color when formatter matches metric
value', async () => {
+ d3Any.json.mockImplementation((_url: string, cb: D3JsonCallback) =>
+ cb(null, mockMapData),
+ );
+
+ const mockFormatters = [
+ {
+ column: 'metric',
+ getColorFromValue: (val: number) =>
+ val >= 100 ? '#FF0000' : undefined,
+ },
+ ];
+
+ render(
+ <ReactCountryMap
+ width={500}
+ height={300}
+ data={[{ country_id: 'CAN', metric: 100 }]}
+ country="canada"
+ linearColorScheme="bnbColors"
+ colorScheme=""
+ formatter={jest.fn().mockReturnValue('100')}
+ formatters={mockFormatters}
+ />,
+ );
+
+ const region = document.querySelector('path.region');
+ expect(region).not.toBeNull();
+ expect(region).toHaveStyle({ fill: '#FF0000' });
+ });
+
+ test('falls back to default color when conditional formatter threshold is
not met', async () => {
+ d3Any.json.mockImplementation((_url: string, cb: D3JsonCallback) =>
+ cb(null, mockMapData),
+ );
+
+ const mockFormatters = [
+ {
+ column: 'metric',
+ getColorFromValue: (val: number) => (val > 500 ? '#FF0000' :
undefined),
+ },
+ ];
+
+ render(
+ <ReactCountryMap
+ width={500}
+ height={300}
+ data={[{ country_id: 'CAN', metric: 100 }]}
+ country="canada"
+ linearColorScheme="bnbColors"
+ colorScheme=""
+ formatter={jest.fn().mockReturnValue('100')}
+ formatters={mockFormatters}
+ />,
+ );
+
+ const region = document.querySelector('path.region');
+ expect(region).not.toBeNull();
+ expect(region).not.toHaveStyle({ fill: '#FF0000' });
+ expect(region).toHaveStyle({ fill: 'rgb(21, 74, 134)' });
Review Comment:
This assertion hard-codes an exact RGB output for the default color scale,
which is likely to be brittle if the `bnbColors` palette or color scaling
implementation changes. A more stable test would derive the expected color from
the same scheme registry/scale used by the component, or assert the fallback
behavior more generally (e.g., it is not the conditional formatting color and
is a non-empty fill).
##########
superset-frontend/plugins/plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -125,11 +128,19 @@ function CountryMap(element: HTMLElement, props:
CountryMapProps) {
? colorScale(d.country_id, sliceId)
: (linearColorScale(d.metric) ?? '');
});
+ const regionMap = new Map(data.map(region => [region.country_id, region]));
const colorFn = (feature: GeoFeature): string => {
if (!feature?.properties) return '#d9d9d9';
- const iso = feature.properties.ISO;
- return colorMap[iso] || '#d9d9d9';
+ const regionData = regionMap.get(feature.properties.ISO);
+
+ if (regionData && formatters?.length > 0) {
+ for (const colorFormatter of formatters) {
+ const cfColor = colorFormatter.getColorFromValue(regionData.metric);
+ if (cfColor) return cfColor;
+ }
+ }
+ return colorMap[feature.properties.ISO] || '#d9d9d9';
};
Review Comment:
The formatter evaluation runs inside `colorFn`, which is called per feature.
With many features and multiple conditional rules, this becomes O(features ×
rules) on each render. Consider precomputing a `conditionalColorByIso` map once
(iterate `data` and apply the formatter chain per row) and then have `colorFn`
do an O(1) lookup before falling back to `colorMap`.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -461,7 +465,7 @@ export const FormattingPopoverContent = ({
</Col>
</Row>
) : null}
- {visibleUseGradient && (
+ {(columnMetricFlag || visibleUseGradient) && (
Review Comment:
When `columnMetricFlag` is true the UI hides the
`columnFormatting`/`objectFormatting` fields, but the form can still carry
previously-initialized values from `initialValues={config}` (AntD Form
preserves unmounted fields by default). This can cause hidden/stale
formatting-target values to be submitted even though the user cannot see/edit
them in metric-only mode. Recommended fix: when `columnMetricFlag` is true,
explicitly clear/omit those fields on submit (e.g., wrap `onFinish` to strip
them), and/or set the hidden Form.Items to `preserve={false}` / reset those
fields in an effect when toggling into metric-only mode.
##########
superset-frontend/plugins/plugin-chart-country-map/src/controlPanel.ts:
##########
@@ -71,6 +73,43 @@ const config: ControlPanelConfig = {
],
['currency_format'],
['linear_color_scheme'],
+ [
+ {
+ name: 'conditional_formatting',
+ config: {
+ type: 'ConditionalFormattingControl',
+ renderTrigger: true,
+ label: t('Custom conditional formatting'),
+ description: t(
+ 'Apply conditional color formatting to numeric columns',
+ ),
+ columnMetricFlag: true,
+ shouldMapStateToProps() {
+ return true;
+ },
+ mapStateToProps(explore, _, chart) {
+ const chartStatus = chart?.chartStatus;
+ const verboseMap = explore?.datasource?.hasOwnProperty(
+ 'verbose_map',
+ )
+ ? (explore?.datasource as Dataset)?.verbose_map
+ : (explore?.datasource?.columns ?? {});
+ const columnOptions = [
+ {
+ value: 'metric',
+ label: 'metric',
+ dataType: GenericDataType.Numeric,
+ },
+ ];
+ return {
+ removeIrrelevantConditions: chartStatus === 'success',
+ columnOptions,
+ verboseMap,
+ };
+ },
Review Comment:
Two concrete issues in the `verboseMap` construction: (1) using
`obj.hasOwnProperty(...)` directly is discouraged (can be shadowed); prefer
`Object.prototype.hasOwnProperty.call(explore?.datasource, 'verbose_map')`. (2)
falling back to `explore?.datasource?.columns` likely yields an array/shape
that is not a `{[key: string]: string}` verbose map, so `verboseMap?.[column]`
will often be undefined. Consider defaulting to `{}` (or building a proper
mapping from `columns` if needed) so label resolution behaves deterministically.
--
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]
