-
Notifications
You must be signed in to change notification settings - Fork 16.3k
feat: Floating Point Formatting for Scatter Point Chart #35915
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
Conversation
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Inefficient linear search in visibility function ▹ view | ||
| Overly permissive numeric type detection ▹ view | ||
| Duplicated Column Type Checking Logic ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx | ✅ |
| superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
Outdated
Show resolved
Hide resolved
Interaction Diagram by BitosequenceDiagram
participant Dev as Developer
participant SharedCtrl as sharedControls.tsx<br/>🟩 Added | ●●● High
participant CtrlPanel as controlPanel.tsx<br/>🔄 Updated | ●●● High
participant Types as types.ts<br/>🔄 Updated | ●●○ Medium
participant Constants as constants.ts<br/>🔄 Updated | ●●○ Medium
participant TransformProps as transformProps.ts<br/>🔄 Updated | ●●● High
participant EchartsUI as EchartsTimeseries Component
Dev->>SharedCtrl: Define x_axis_number_format control
SharedCtrl-->>CtrlPanel: Export new control config
CtrlPanel->>CtrlPanel: Add visibility logic for numeric X-axis
CtrlPanel->>Types: Pass xAxisNumberFormat field
Types->>Constants: Initialize with SMART_NUMBER default
Constants->>TransformProps: Provide default format value
TransformProps->>TransformProps: Check if X-axis is Numeric type
TransformProps->>TransformProps: Apply getNumberFormatter(xAxisNumberFormat)
TransformProps-->>EchartsUI: Return formatted echartOptions
Critical path: Developer->sharedControls.tsx->controlPanel.tsx->transformProps.ts->EchartsTimeseries Component
|
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 #2e97c7
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx - 1
- Incorrect time format visibility for DATE columns · Line 127-127
Review Details
-
Files reviewed - 5 · Commit Range:
0fab5cb..f6a95e7- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- 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/types.ts
-
Files skipped - 0
-
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 evan@preset.io.
Documentation & Help
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
Outdated
Show resolved
Hide resolved
|
@alex241728 thanks for creating the PR. Do you mind adding tests to your new feature? |
|
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
f6a95e7 to
a342a3d
Compare
|
Re-running CI 🤞 |
b6069e4 to
91c3f48
Compare
I have completed the tests and rerun the precommit checks. I think the tests and checks are all passed. Can you check the pull request again. @rusackas |
|
For tests, we should move away from |
090954e to
d4497df
Compare
@sadpandajoe @rusackas The test format has been fixed. |
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 adds floating-point number formatting support for scatter chart x-axes, addressing the limitation where only time formatting was previously available. The implementation introduces a new xAxisNumberFormat control that appears when a numeric column (specifically floating-point types like FLOAT, DOUBLE, REAL, NUMERIC, DECIMAL) is selected for the x-axis.
Key Changes:
- Added conditional number formatting for x-axis based on data type (numeric vs temporal)
- Introduced new
x_axis_number_formatcontrol with visibility logic tied to column data types - Default number format set to 'SMART_NUMBER' for numeric x-axes
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
transformProps.test.ts |
Added comprehensive tests for both time and number formatting on scatter chart x-axes |
controlPanel.test.ts |
Added tests validating visibility logic for time and number format controls |
types.ts |
Extended form data type definition to include xAxisNumberFormat property |
transformProps.ts |
Implemented conditional formatter selection based on x-axis data type (Temporal vs Numeric) |
constants.ts |
Added default value 'SMART_NUMBER' for xAxisNumberFormat |
controlPanel.tsx |
Added visibility-controlled UI controls for time and number formatting with data-type-specific logic |
sharedControls.tsx |
Defined reusable x_axis_number_format control configuration |
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts
Outdated
Show resolved
Hide resolved
|
Looking at some of the failing CI stuff, this PR might need a rebase, pulling in the latest changes from |
30c5b29 to
33df01b
Compare
@rusackas I have solved the suggestions provided by copilot. However, there is one thing I cannot solve in the pre-commit checks, which is provided in the quote. By the way, the transformProps.test for bar chart contains the same superTheme from the same file. It has the type error too.
|
33df01b to
feeb12a
Compare
The error is fixed by
|
|
🎪 Showtime deployed environment on GHA for feeb12a • Environment: http://35.89.8.79:8080 (admin/admin) |
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.
Looks good to me! Thanks for the contribution!
Co-authored-by: Vincent <vincent.trung4@gmail.com>
Co-authored-by: Vincent <vincent.trung4@gmail.com>
SUMMARY
According to the issue #32902, the scatter point charts lack of generic axes, which means the scatter point charts only allow x-axis to be formatted with time formats. So, to fix the problem mentioned in this issue, our team adds a new floating-point formats for x-axis.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
The before screenshot can be checked in issue #32902
The after screenshot:

TESTING INSTRUCTIONS
ADDITIONAL INFORMATION