rebenitez1802 commented on PR #39451:
URL: https://github.com/apache/superset/pull/39451#issuecomment-4960176177
**Request changes (non-blocking):** the class→function conversions are
correct, but the bundled `theme={supersetTheme}` additions contradict the "pure
structural change, no functional differences" claim and should be split out or
documented.
🟡 **Medium — `theme={supersetTheme}` is a functional change mislabeled as
structural**
The PR adds `import { supersetTheme }` + `theme={supersetTheme}` to
`<SuperChart>` in ~35 files (64 spots), but the title is `chore(lint)` and the
body says "pure structural change, no functional differences." Per
`SuperChart.tsx:134-142`, theme resolves as `themeProp ?? (contextTheme ??
supersetTheme)` — the explicit prop **wins over context**. So where a Storybook
`ThemeProvider`/decorator supplies a theme, these charts now ignore it and are
pinned to the light default; it's only a no-op when no context exists. This is
real runtime behavior, unrelated to the class→function work. **Fix:** either
split the theme changes into their own PR, or call them out explicitly in the
description. Prefer relying on a global Storybook `ThemeProvider` decorator
over per-chart hardcoded `theme` props so stories still honor theme switching
(dark mode).
🟢 **Low — `Expandable` label output changed (a beneficial fix, but not "pure
structural")**
Original always rendered `` `${open ? 'Hide' : 'Show'} ${expandableWhat}`
``, so `expandableWhat === undefined` produced the literal `"Show undefined"`.
The new code branches and renders just `"Show"`/`"Hide"` when absent.
Byte-identical for non-empty strings and clearly an improvement, but it *is* an
observable output change. Worth a one-line note in the PR and a check that no
story/snapshot asserts the old text.
🟢 **Low — `theme` hardcoding forecloses theme switching in stories**
Even where harmless, pinning every chart to `supersetTheme` is the wrong
pattern for Storybook — a theme decorator can never override it. Rely on the
context fallback instead of passing the prop.
---
**Verified correct (no action needed):**
- `VerifyCORS.tsx` — the trickiest conversion. The
`componentDidUpdate`→`useEffect` + `prevPropsRef` port is behaviorally
equivalent: no reset on mount, no infinite loop from the `didVerify`/`error`
deps, `handleVerify` deps complete, and `<>{children({ payload })}</>`
correctly satisfies the `JSX.Element` return type with no DOM impact.
- `NumberFormat` / `TimeFormat` stories — hoisting the never-mutated
`testValues` to a module-level const is equivalent; `useCallback([])` and
`key={index}` (static list) are fine.
- The ~35 theme-prop-only files are otherwise clean pure additions; import
path `@apache-superset/core/theme` and prop placement are consistent.
Net: mergeable once the `theme` scope creep is either separated out or
explicitly documented — the conversions themselves are solid.
<sub>🤖 Reviewed with a multi-agent team via Claude Code</sub>
--
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]
