endimonan commented on PR #44790:
URL: https://github.com/apache/superset/pull/44790#issuecomment-5998315539
> Richard's agent here: thanks for tackling this. Keeping SQL NULL and the
literal `N/A` apart is a real fix. One design concern before approving
(reviewed at `18e594d`):
>
> **Null color now diverges from other ECharts plugins.** Pie, Bubble and
Timeseries (via `formatSeriesName`) key a null as `<NULL>` and color it through
`colorFn('<NULL>', sliceId)`, so `label_colors: {"<NULL>": …}` and the shared
dashboard color map apply to nulls. With this PR, Graph shows `<NULL>` for
nulls but colors them per chart, outside the shared map, and
`label_colors["<NULL>"]` applies to the literal string instead (the test at
`categoryIdentity.test.ts:136` asserts this). On one dashboard, a Pie and a
Graph over the same column would show the same `<NULL>` label in different
colors. Existing dashboards that set `{"N/A": …}` to color Graph nulls would
also lose that color with no way to replace it.
>
> Would you consider swapping the escaping? Keep null keyed as `NULL_STRING`
like the other plugins, and give the literal `"<NULL>"` the escaped key and
quoted display. That matches #43547's push for consistent null handling. It
should also remove most of the sentinel machinery: the reserved-prefix
escaping, the free-key loop for the null color, the literal pre-pass, and the
two color lookup paths. The new tests could then shrink considerably.
>
> Minor: the docs paragraph lands inside the Tutorial Dashboard walkthrough
in `exploring-data.mdx`. A sentence on how Graph shows `<NULL>` is probably
enough there.
Thanks Richard, I switched Graph to the shared <NULL> color and kept literal
values separate with quoted labels.
I also shortened the docs and added regression tests; the related tests,
browser checks, and pre-commit all pass
--
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]
