kgabryje commented on PR #36144:
URL: https://github.com/apache/superset/pull/36144#issuecomment-4487932543
## TL;DR — Suggest not merging as-is
Thanks for the cleanup pass! Unfortunately, as written, this introduces more
issues than it fixes. Flagging the concrete problems below so they can be
addressed in a follow-up.
---
## Critical issues
### 1. Both Python examples are missing the closing `}` for `THEME_DEFAULT`
- **Example 1** (around L229-256): `THEME_DEFAULT = {` opens, then three
closing braces close `legend`/`echartsOptionsOverrides`/`token`, then the code
fence — `THEME_DEFAULT` is never closed → `SyntaxError`.
- **Example 2** (around L262-305): same pattern, also missing the outer
closing brace.
A user copy-pasting either snippet will get a syntax error before anything
else runs.
### 2. `echartsOptionsOverrides` nested inside `token` contradicts the parser
The PR moves `echartsOptionsOverrides` and
`echartsOptionsOverridesByChartType` from top-level (sibling of `token`) into
`token`. The parser explicitly does the opposite — see
[`superset-frontend/packages/superset-core/src/theme/Theme.tsx#L127-L134`](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-core/src/theme/Theme.tsx#L127-L134):
```ts
// Extract Superset-specific properties from top-level config.
// These are custom properties that aren't part of Ant Design's token system
// but need to be passed through to the SupersetTheme for ECharts
customization.
const { echartsOptionsOverrides, echartsOptionsOverridesByChartType } =
config as AnyThemeConfig & { ... };
```
The unit tests in `Theme.test.tsx` are literally named [`"Theme includes
echartsOptionsOverrides from top-level
config"`](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-core/src/theme/Theme.test.tsx#L770)
and place the keys as siblings of `token`. It might happen to work
coincidentally (because `antdConfig.token` is spread into the theme), but
that's a side effect, not the documented or tested API — and the parser comment
explicitly says these are *not* part of the Ant Design token system.
### 3. `"show": True` → `"show": "True"` is a regression (around L291)
- Original Python `True` (a real boolean) — correct inside a ` ```python `
block; ECharts gets a bool.
- New `"True"` is a **string**. `dataZoom[].show` expects a boolean.
If the goal was JSON validity, the fix would be lowercase `true` — but only
in JSON examples; the Python `True` was fine. (Bito's bot flagged this one too
on commit `22bbcb1`.)
---
## Partial / inconsistent fixes
### 4. `echarts_pie` → `pie` is correct, but the rest of the chart-type list
is still wrong
`VizType.Pie = 'pie'` in
[`VizType.ts#L49`](https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/chart/types/VizType.ts#L49)
— so the rename is right. But the surrounding list still has many incorrect
identifiers, which the renderer uses as direct lookup keys at
[`Echart.tsx#L262`](https://github.com/apache/superset/blob/master/superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx#L262):
| Doc says | Actual VizType value |
|---|---|
| `echarts_timeseries` | `echarts_timeseries` ✓ |
| `pie` (new) | `pie` ✓ |
| `echarts_bubble` | `bubble_v2` |
| `echarts_funnel` | `funnel` |
| `echarts_gauge` | `gauge_chart` |
| `echarts_radar` | `radar` |
| `echarts_boxplot` | `box_plot` |
| `echarts_treemap` | `treemap_v2` |
| `echarts_sunburst` | `sunburst_v2` |
| `echarts_graph` | `graph_chart` |
| `echarts_sankey` | `sankey_v2` |
| `echarts_heatmap` | `heatmap_v2` |
| `echarts_mixed_timeseries` | `mixed_timeseries` |
Example 2 also still uses `"echarts_bubble"` as a key inside the override
map — that lookup will silently never match.
### 5. Indentation is inconsistent across the file
- Example 1: 2-space.
- Example 2: outer 4-space, inner restructured block 2-space — mixed within
one snippet.
- Corporate Branding example: 2-space.
- Array Property Overrides example: 4-space.
PEP 8 is 4-space; if reformatting, apply it uniformly.
### 6. Stray blank line inside the last code block before the closing fence
(cosmetic).
---
## Suggested direction
If the underlying intent is "the docs have some wrong info" (which is true),
the fixes are:
1. **Revert the nesting** — keep `echartsOptionsOverrides` /
`echartsOptionsOverridesByChartType` as **top-level** keys, siblings of `token`.
2. **Fix the full chart-type table** against `VizType.ts`, plus the
`echarts_bubble` key inside Example 2.
3. **Restore Python `True`** in `python` blocks; use lowercase `true` only
in the JSON example.
4. **Don't drop the closing `}`** on `THEME_DEFAULT` in either Python
example.
5. If reformatting indentation, do the whole file consistently.
---
## Non-blocking notes
- All CI checks pass, but they don't validate that mdx code fences parse,
nor that example schemas match the parser — green CI isn't evidence of
correctness here.
- Korbit's approval is low-signal: it scans a non-existent path
`docs/docs/configuration/theming.mdx` (the file actually lives at
`docs/admin_docs/...`).
--
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]
