feat(scatter): add chart orientation and dot size metric controls#40967
Open
rusackas wants to merge 4 commits into
Open
feat(scatter): add chart orientation and dot size metric controls#40967rusackas wants to merge 4 commits into
rusackas wants to merge 4 commits into
Conversation
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Contributor
|
The architect review correctly identifies that time-comparison size series (e.g., Proposed FixIn // Example of updated detection logic
const isSizeSeries = (seriesName: string, sizeMetric: string) => {
const basePattern = `^${sizeMetric}(, .*)?$`;
const timeComparisonPattern = `^${sizeMetric}__.*(, .*)?$`;
return new RegExp(`${basePattern}|${timeComparisonPattern}`).test(seriesName);
};This change ensures that both standard size metrics and their time-comparison variants are correctly identified and excluded from superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40967 +/- ##
==========================================
+ Coverage 64.29% 64.31% +0.01%
==========================================
Files 2659 2659
Lines 144266 144381 +115
Branches 33247 33305 +58
==========================================
+ Hits 92754 92854 +100
- Misses 49881 49896 +15
Partials 1631 1631
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #1871ad
Actionable Suggestions - 3
-
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts - 1
- Duplicate test control setup across test files · Line 27-27
-
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts - 2
- Test validates non-existent code · Line 49-56
- Test validates non-existent dedup logic · Line 58-65
Additional Suggestions - 2
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx - 1
-
Axis controls lose orientation awareness · Line 475-476Please move `truncateXAxis` and `xAxisBounds` into the array returned by `createAxisControl('x')` so that they inherit the `showsDimensionAxis`/`showsMetricAxis` visibility logic.
-
-
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts - 1
-
Missing describe block grouping · Line 1603-1627The new tests for `getAreaScaledSymbolSize` are not wrapped in a `describe` block, unlike most other tests in this file (10 describe blocks exist). Grouping related tests improves test organization and enables running subsets with `jest --testNamePattern`.
-
Review Details
-
Files reviewed - 10 · Commit Range:
1fbde07..1fbde07- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ 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 Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
rusackas
force-pushed
the
feat/scatter-categorical-y-and-dot-size
branch
from
1fbde07 to
334ed5b
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #b98d61
Actionable Suggestions - 2
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts - 1
- Missing unit tests for symbolSizeFn · Line 208-208
-
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts - 1
- Consolidate duplicate mockControls utility across chart test files · Line 48-48
Additional Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx - 1
-
Asymmetric axis truncation controls · Line 475-476`[truncateXAxis]` and `[xAxisBounds]` are placed after `createAxisControl('x')` (lines 475-476) but no equivalent `truncateYAxis`/`yAxisBounds` controls appear after `createAxisControl('y')` (line 480). This creates an asymmetry where X Axis truncation/bounds are configurable but Y Axis truncation/bounds are only available inside `createAxisControl('y')` when orientation switches them to the metric axis.
-
Review Details
-
Files reviewed - 10 · Commit Range:
334ed5b..0d617ff- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ 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 Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Adds two capabilities to the ECharts Scatter chart (echarts_timeseries_scatter): - A Vertical/Horizontal orientation control, reusing the shared Timeseries horizontal-orientation plumbing built for the Bar chart. Horizontal orientation places the dimension on the y-axis, enabling categorical (or time/numeric) y-axes with the metric on the x-axis. Axis-related controls swap between the X/Y sections based on orientation, mirroring the Bar chart panel. - An optional "Dot size metric" with minimum/maximum dot size sliders. The size metric rides along in the query (deduped if it matches a value metric), its series are excluded from rendering, and each point's marker is sized so that marker *area* scales linearly with the metric between the configured bounds, avoiding the perceptual exaggeration of diameter-linear scaling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…op any types in tests - normalize an inverted min/max dot size range so larger values always render as larger dots - fall back to the minimum configured dot size (not the hidden fixed marker size control) for points missing a size value - replace any annotations/casts in scatter control panel and transformProps tests with concrete test types Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…helpers - Add transformSeries unit tests for the symbolSizeFn / markerSize fallback - Extract duplicated getControl/mockControls helpers from five Timeseries controlPanel test files into the shared test helpers module Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
feat/scatter-categorical-y-and-dot-size
branch
from
0d617ff to
94262b7
Compare
…ze assertions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Code Review Agent Run #716fc2Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Adds two long-requested capabilities to the ECharts Scatter chart (
echarts_timeseries_scatter):1. Chart orientation (categorical y-axis support)
A new Vertical/Horizontal orientation control, reusing the shared Timeseries horizontal-orientation plumbing that the Bar chart already uses. Horizontal orientation places the dimension (x-axis column) on the y-axis and the metric on the x-axis — so a categorical (or temporal/numeric) y-axis is one click away, and combined with the existing "Force categorical" control even numeric columns can be treated as categories. Axis controls (formats, label rotation, log axis, bounds, titles) swap between the X Axis and Y Axis panel sections based on orientation, mirroring the Bar chart's control panel behavior.
2. Dot size metric with area-based scaling
An optional "Dot size metric" control in the Query section, plus "Minimum dot size" / "Maximum dot size" sliders (defaults 5/30) that appear when the metric is set. Design notes:
size → metricsquery-field alias, so nobuildQuerychange was needed; it is deduped when it matches a value metric (in which case each point's own value doubles as its size value).symbolSizecallbacks, matched to value series by dimension key for grouped charts.getAreaScaledSymbolSizeinutils/series.ts): marker area scales linearly with the metric between the configured bounds, i.e. radius ∝ √value. This avoids the perceptual exaggeration of diameter-linear scaling (where a 2× value reads as a 4× area) — notably the existing Bubble chart scales linearly on radius.Both features compose (horizontal orientation + sized dots) and are covered by unit tests.
Possible follow-ups (intentionally out of scope): showing the size value in the tooltip, and adopting area-based scaling in the Bubble chart.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: the Scatter chart only offered a fixed marker-size slider and a vertical layout.
After: orientation radio control under "Chart Orientation", "Dot size metric" in the Query section, and min/max dot size sliders under Chart Options.
TESTING INSTRUCTIONS
COUNT(*)); dots resize so their areas are proportional to the metric, scaled between the Minimum/Maximum dot size sliders that appear under Chart Options.npx jest plugins/plugin-chart-echarts/test/Timeseries plugins/plugin-chart-echarts/test/utils/series.test.tsADDITIONAL INFORMATION
🤖 Generated with Claude Code