-
Notifications
You must be signed in to change notification settings - Fork 16.2k
refactor(word-cloud): convert rotation and color controls to React components (POC) #36275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
refactor(word-cloud): convert rotation and color controls to React components (POC) #36275
Conversation
|
@rusackas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #34e1ef
Actionable Suggestions - 5
-
superset-frontend/src/explore/controlUtils/getControlState.ts - 1
- Violation of TypeScript `any` prohibition · Line 185-192
-
superset-frontend/src/explore/components/ControlPanelsContainer.tsx - 1
- TypeScript `any` type usage violation · Line 663-683
-
superset-frontend/plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx - 2
- Wrong test assertion for select presence · Line 40-40
- Incomplete onChange test · Line 43-43
-
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/ColorSchemeLabel.tsx - 1
- Tooltip content not rendering due to missing function call · Line 83-83
Review Details
-
Files reviewed - 10 · Commit Range:
8a5c6a5..3dead35- superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl.tsx
- superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/ColorSchemeLabel.tsx
- superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/index.tsx
- superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/RotationControl.tsx
- superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/index.ts
- superset-frontend/plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx
- superset-frontend/plugins/plugin-chart-word-cloud/test/controlPanel.test.ts
- superset-frontend/src/explore/components/ControlPanelsContainer.tsx
- superset-frontend/src/explore/controlUtils/getControlState.ts
- superset-frontend/src/utils/getControlsForVizType.ts
-
Files skipped - 1
- superset-frontend/plugins/plugin-chart-word-cloud/test/tsconfig.json - Reason: Filter setting
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
superset-frontend/src/explore/components/ControlPanelsContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx
Outdated
Show resolved
Hide resolved
.../plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/ColorSchemeLabel.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes the Word Cloud chart plugin's control panel by converting two key controls from config-object declarations to standalone React components, serving as a proof-of-concept for the broader control panel refactor initiative.
Key Changes
- Introduced
RotationControlandColorSchemeControlas React components with proper prop handling and live update support viarenderTrigger - Enhanced
ControlPanelsContainerto inject props (value,onChange,actions, etc.) into React element controls usingcloneElement - Extended utility functions (
getControlState,getControlsForVizType) to recognize and process React elements alongside traditional config objects
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controlPanel.tsx |
Replaced config-based rotation and color_scheme controls with React component instances |
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/RotationControl.tsx |
New React component for rotation control with Select dropdown and proper change handlers |
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl.tsx |
Wrapper component that integrates the internal ColorSchemeControl for plugin use |
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/index.tsx |
Comprehensive color scheme control component ported from Explore with dashboard context support |
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/ColorSchemeLabel.tsx |
UI component for rendering color scheme labels with color swatches |
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/index.ts |
Export barrel for the new control components |
superset-frontend/src/explore/components/ControlPanelsContainer.tsx |
Added logic to clone React element controls with injected props and state bindings |
superset-frontend/src/explore/controlUtils/getControlState.ts |
Extended to extract control state from React element props |
superset-frontend/src/utils/getControlsForVizType.ts |
Added support for mapping React element controls to control metadata |
superset-frontend/plugins/plugin-chart-word-cloud/test/controlPanel.test.ts |
New test verifying React controls are present in control panel sections |
superset-frontend/plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx |
New test suite for RotationControl component |
superset-frontend/plugins/plugin-chart-word-cloud/test/tsconfig.json |
Removed unnecessary rootDir configuration |
...et-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/RotationControl.tsx
Show resolved
Hide resolved
superset-frontend/src/explore/components/ControlPanelsContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/ControlPanelsContainer.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx
Outdated
Show resolved
Hide resolved
...et-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/index.tsx
Outdated
Show resolved
Hide resolved
...et-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/index.tsx
Outdated
Show resolved
Hide resolved
| }, | ||
| }, | ||
| ], | ||
| [<RotationControl name="rotation" key="rotation" renderTrigger />], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me wonder something about our general pattern here.
In this instance, we're all good, I think, since the props/values for this component is always going to have the same options for the dropdown, and will always be visible (since it's always relevant to the component).
But, let's say we're adding a React control for something more dynamic... like the "Radius" control in the Pie chart. That one should only be rendered/displayed if the "Donut" checkbox is checked. So, let's say you set the radius control to a value, uncheck "Donut" and then check "Donut" again. Would the value persist? If not, maybe we should consider adding a "hide" prop that removes input visibility without un-rendering it.
Also, there's a likelihood that SOME controls will have dynamic values passed in as props (based on other controls in the control panel). In those cases more than this one, we'll probably want to memoize more. Memoization doesn't seem necessary so much in this particular control, but I'm wondering if we should just memoize things anyway, to establish a pattern for copypasting our way through all control migrations. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m leaning towards not using a blanket memoization approach. Looking at the codebase, React.memo is only used in 2 controls (FastVizSwitcher, ColumnConfigItem), both with clear performance needs. The implicit pattern seems to be: only memoize when there's a demonstrated need.
For controls with dynamic props and computed options, we should use useMemo/useCallback for those computed values, but not necessarily wrap the whole component in React.memo unless we see performance issues.
Let's keep this simple and address performance on a case-by-case basis as we encounter more complex controls during migration.
rusackas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great, and is a small but critical step toward better control panels. My only questions/criticisms are thinking ahead a bit, but we can address these things as we start to spread this new pattern around.
|
Re-running a failing test... could be flaky, may need addressing. Also, there are a lot of open bot comments... would you mind either addressing them, or leaving some "not relevant" kind of comment on them? |
SUMMARY
This PR is a first, targeted step toward the long‑term control panel revamp described in #35655: moving away from string/config‑driven controls toward explicit React components.
Concretely, for the Word Cloud plugin:
controlPanel.tsxwith a dedicatedRotationControlReact component, and wired it throughControlPanelsContainerso it receivesvalue/onChangeand usesrenderTriggerfor live updates.color_schemestring reference with aColorSchemeControlWrapperReact component, backed by a local copy of the ExploreColorSchemeControl. This keeps the plugin self‑contained while letting the control behave like any other React component (props,onChange,renderTrigger).controlPanelSections/controlSetRowsstructure intact (per the “start small” guidance), but proved that individual controls can be swapped from config/string references to React components without breaking existing behavior.RotationControland for the Word CloudcontrolPanelto ensure the new React controls are present in theOptionssection and correctly wired.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable – this change affects Explore control behavior, not the visual layout.
TESTING INSTRUCTIONS
Install frontend deps (if needed)
cd superset-frontend
npm install
2. Frontend type checking (includes Word Cloud plugin)
From the repo root:
pre-commit run --all-files type-checking-frontend
3. Run Word Cloud plugin tests
From
superset-frontend:npm run test -- plugins/plugin-chart-word-cloud/test/RotationControl.test.tsx
npm run test -- plugins/plugin-chart-word-cloud/test/controlPanel.test.ts
4. Manual verification in Explore
ADDITIONAL INFORMATION
(Explore behavior for Word Cloud changes: rotation and color scheme now live‑update.)