sadpandajoe commented on code in PR #45585:
URL: https://github.com/apache/superset/pull/45585#discussion_r4351908440
##########
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 sentinel is only formatted here, in the default category row.
`createHandlebarsTooltipData` still spreads the raw feature, so a custom
tooltip template with `{{cat_color}}` will show `<NULL>` for a null-category
point, while the legend shows N/A for the same point. These points were
previously dropped, so this is newly reachable. Should the handlebars data
apply `formatNullCategory` to `cat_color` too?
##########
superset-frontend/plugins/preset-chart-deckgl/src/CategoricalDeckGLContainer.test.tsx:
##########
@@ -275,3 +276,25 @@ test('with autozoom a tilt written back by Explore keeps
the viewer on the same
);
expect(mockViewports.at(-1)).toEqual(fitted);
});
+
+test('rows with a null dimension get their own legend category', () => {
+ const fd = {
+ datasource: '1__table',
+ viz_type: 'deck_scatter',
+ color_scheme_type: COLOR_SCHEME_TYPES.categorical_palette,
+ color_scheme: 'supersetColors',
+ dimension: 'category',
+ slice_id: 42,
+ color_picker: { r: 0, g: 0, b: 0, a: 1 },
+ } as unknown as QueryFormData;
+ const data = [{ cat_color: 'A' }, { cat_color: NULL_CATEGORY_KEY }];
+
+ const categories = getCategories(fd, data);
+
+ expect(categories[NULL_CATEGORY_KEY]?.enabled).toBe(true);
+ expect(categories[NULL_CATEGORY_KEY]?.color).toBeDefined();
+
+ // Without a category entry the container drops the feature entirely, which
is
+ // how null-dimension rows used to disappear from the map.
+ expect(data.filter(d => categories[d.cat_color]?.enabled)).toHaveLength(2);
Review Comment:
This test feeds `getCategories` an already-normalized `<NULL>` key and then
repeats the container's legend filter inline, so it would still pass if the
container or a layer dropped null rows. Nothing renders
`CategoricalDeckGLContainer` with real `transformProps` output for mixed
`'A'`/null rows. Could you add a render test asserting both rows reach the
mocked layer with the N/A category color, and that toggling N/A in the legend
removes only the null row?
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Scatter/transformProps.ts:
##########
@@ -155,9 +156,10 @@ function processScatterData(
}
}
- const categoryValue = getSourceValue(categoryColumn);
- if (categoryValue != null) {
- scatterPoint.cat_color = String(categoryValue);
+ if (categoryColumn) {
+ const categoryValue = getSourceValue(categoryColumn);
+ scatterPoint.cat_color =
Review Comment:
The sentinel is assigned before `addPropertiesToFeature` runs, so a selected
column literally named `cat_color` (e.g. one added for tooltips) is copied over
it. With a null dimension value and a null `cat_color` column, the feature ends
up with `cat_color: null`, which `getCategories` skips and the legend filter
drops, so that point still disappears. Arc was reordered to assign after the
copy for this reason; Path (`layers/Path/transformProps.ts`, the
`feature.cat_color =` block) has the same ordering. Should these two assign
after the copy as well?
--
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]
