sadpandajoe commented on code in PR #45585:
URL: https://github.com/apache/superset/pull/45585#discussion_r4365645518
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Scatter/transformProps.test.ts:
##########
@@ -197,6 +198,49 @@ test('Scatter transformProps should handle dimension for
category colors', () =>
expect(features[1]?.cat_color).toBe('B');
});
+test('Scatter transformProps should fall back to NULL_CATEGORY_KEY for empty
dimension values', () => {
+ const propsWithNullCategory = {
+ ...mockChartProps,
+ rawFormData: {
+ ...mockChartProps.rawFormData,
+ dimension: 'category',
+ point_radius_fixed: {
+ type: 'fix',
+ value: '1000',
+ },
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ LATITUDE: 37.8,
+ LONGITUDE: -122.4,
+ category: 'A',
+ },
+ {
+ LATITUDE: 37.9,
+ LONGITUDE: -122.3,
+ category: null,
+ },
+ {
+ LATITUDE: 38.0,
+ LONGITUDE: -122.2,
+ category: undefined,
+ },
+ ],
+ },
+ ],
+ };
+
+ const result = transformProps(propsWithNullCategory as ChartProps);
+ const features = result.payload.data.features as ScatterFeature[];
+
+ expect(features).toHaveLength(3);
+ expect(features[0]?.cat_color).toBe('A');
+ expect(features[1]?.cat_color).toBe(NULL_CATEGORY_KEY);
Review Comment:
The ordering fix (assigning `cat_color` after `addPropertiesToFeature`) has
no test: none of the Scatter, Arc or Path fixtures include a source column
literally named `cat_color`, so moving the assignment back above the property
copy would leave every test here green while null-dimension points vanish
again. Could you add a row like `{ category: null, cat_color: 'unrelated' }`
and assert the feature's `cat_color` is `NULL_CATEGORY_KEY`, in each of the
three transforms?
##########
superset-frontend/plugins/preset-chart-deckgl/src/utilities/tooltipUtils.tsx:
##########
@@ -58,7 +59,7 @@ export const CommonTooltipRows = {
o.object?.cat_color ? (
<TooltipRow
label={`${t('Category')}: `}
- value={`${o.object.cat_color}`}
+ value={formatNullCategory(`${o.object.cat_color}`)}
Review Comment:
The default category row is fixed, but a null-category point can still show
`<NULL>` in the tooltip: when `cat_color` is one of the configured tooltip
contents, the merge of `tooltipData` near the end of
`createHandlebarsTooltipData` replaces the normalized `N/A` with the raw
feature value, so `{{cat_color}}` still disagrees with the legend. Could the
formatting be applied after that merge instead?
--
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]
