sadpandajoe commented on code in PR #45585:
URL: https://github.com/apache/superset/pull/45585#discussion_r4366610158
##########
superset-frontend/plugins/preset-chart-deckgl/src/utilities/tooltipUtils.tsx:
##########
@@ -348,6 +349,12 @@ export function createHandlebarsTooltipData(
data = { ...data, ...tooltipData };
}
+ // Applied after the tooltip-contents merge so a configured `cat_color` item
+ // cannot reintroduce the raw null sentinel.
+ if (data.cat_color != null) {
+ data = { ...data, cat_color: formatNullCategory(`${data.cat_color}`) };
Review Comment:
This now rewrites `cat_color` for every chart that reaches this function,
not only the null sentinel. For a Screengrid chart with no dimension and
`tooltip_contents` containing a real column named `cat_color`,
`processTooltipContentItem` sets `cat_color` to the array `['A', 'B']`; the
template literal turns it into the string `'A,B'`, so `{{first cat_color}}`
renders nothing and `{{#each cat_color}}` iterates characters. Numeric values
also become strings. Could the replacement only happen when the value is
`NULL_CATEGORY_KEY` and otherwise leave the value untouched?
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -67,7 +67,7 @@ function setTooltipContent(
{o.object?.cat_color && (
<TooltipRow
label={`${t('Category')}: `}
- value={`${o.object?.cat_color}`}
+ value={formatNullCategory(`${o.object?.cat_color}`)}
Review Comment:
Scatter has its own default tooltip renderer, and nothing in the tests
exercises it: the new `CommonTooltipRows.category` test covers a different
function, and there is no `Scatter.test.tsx`. If this `formatNullCategory` call
were reverted, every test would stay green while hovering a null-category point
showed `Category: <NULL>`. Could you add a `getLayer` test with no tooltip
template or contents that invokes `onHover` with a picked feature whose
`cat_color` is the sentinel, renders the captured tooltip, and asserts `N/A`
appears and `<NULL>` does not?
##########
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 formatted here and in the Handlebars path, but the
field-based branch (`buildFieldBasedTooltipItems`, reached when
`tooltip_contents` is set and the template is empty) still passes the raw value
through `formatValue`. With a Scatter, Arc or Path chart that has a nullable
dimension and `cat_color` in Tooltip contents, a null row shows `N/A` in the
legend but `cat_color: <NULL>` on hover. Should that branch run the value
through `formatNullCategory` too?
--
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]
