chore(timeseries charts): adjust legend width by padding #32030
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Unexplained magic numbers in legend width calculation ▹ view | |
 |
Legend width calculation hardcoded values ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-chart-controls/src/types.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| @@ -446,6 +447,10 @@ export function getLegendProps( | |||
| switch (orientation) { | |||
| case LegendOrientation.Left: | |||
| legend.left = 0; | |||
| legend.textStyle = { | |||
| overflow: 'truncate', | |||
| width: Math.max((padding?.left || 205) - 45, 10), | |||
There was a problem hiding this comment.
Unexplained magic numbers in legend width calculation 
Tell me more
What is the issue?
Magic numbers (205, 45, 10) are used in the width calculation without explanation of their significance.
Why this matters
Future maintainers won't understand what these numbers represent or why they were chosen, making it difficult to modify or debug the logic.
Suggested change ∙ Feature Preview
// Extract magic numbers into named constants at the top of the file
const DEFAULT_LEGEND_WIDTH = 205;
const LEGEND_ICON_WIDTH = 45;
const MIN_LEGEND_TEXT_WIDTH = 10;
// Then use in the code
Math.max((padding?.left || DEFAULT_LEGEND_WIDTH) - LEGEND_ICON_WIDTH, MIN_LEGEND_TEXT_WIDTH)
💬 Chat with Korbit by mentioning @korbit-ai.
| legend.textStyle = { | ||
| overflow: 'truncate', | ||
| width: Math.max((padding?.left || 205) - 45, 10), | ||
| }; |
There was a problem hiding this comment.
Legend width calculation hardcoded values 
Tell me more
What is the issue?
The legend text width calculation has a hardcoded fallback value of 205 and a fixed subtraction of 45 pixels, which may not be suitable for all chart sizes and configurations.
Why this matters
Using fixed values for layout calculations can lead to text overflow or underutilized space in different chart sizes and configurations. This reduces the flexibility and responsiveness of the chart legend.
Suggested change ∙ Feature Preview
Use dynamic values based on the chart container size or make these values configurable through the chart configuration. Example:
legend.textStyle = {
overflow: 'truncate',
width: Math.max((padding?.left || padding?.default || containerWidth * 0.2) - padding?.textMargin || 10, 10),
};💬 Chat with Korbit by mentioning @korbit-ai.
eschutho
force-pushed
the
elizabeth/fix-legend-overflow
branch
from
b3b0cc2 to
dfcee49
Compare
eschutho
changed the title
eschutho
changed the title
eschutho
force-pushed
the
elizabeth/fix-legend-overflow
branch
from
dfcee49 to
033bfd9
Compare
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
|
Zero-width legend risk ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-chart-controls/src/types.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | ✅ |
| superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability and Maintainability ✅ Design Patterns ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| @@ -443,13 +444,30 @@ export function getLegendProps( | |||
| borderColor: theme.colors.grayscale.base, | |||
| }, | |||
| }; | |||
| const MIN_LEGEND_WIDTH = 0; | |||
There was a problem hiding this comment.
Zero-width legend risk
Tell me more
What is the issue?
Setting MIN_LEGEND_WIDTH to 0 could cause legends to disappear completely when padding minus MARGIN_GUTTER is 0 or negative.
Why this matters
When the padding is less than or equal to MARGIN_GUTTER (45px), the legend width will be 0, making the legend text invisible and breaking the layout functionality intended by the developer.
Suggested change ∙ Feature Preview
Set a reasonable minimum width to ensure legends remain visible and functional:
const MIN_LEGEND_WIDTH = 100; // or another appropriate minimum width value💬 Chat with Korbit by mentioning @korbit-ai.
| const MIN_LEGEND_WIDTH = 0; | ||
| const MARGIN_GUTTER = 45; | ||
| const getLegendWidth = (paddingWidth: number) => | ||
| Math.max(paddingWidth - MARGIN_GUTTER, MIN_LEGEND_WIDTH); |
There was a problem hiding this comment.
I kinda agree with bot's comment above - maybe we should set a minimal width value where the legend is still readable?
There was a problem hiding this comment.
Yeah, I tried out a few different min widths, but the problem is that when applying a min width, then we have the same overlap issue with the chart. So either way, there will be a UI issue, and a larger padding will need to be set. Someone needs to explicitly set a 0 padding in order for this to apply. If you think having the overlap is better than a readable legend, we can def do that.
Oh, good eye. That was just because I had set the margin to 200 in the video. This is the margin top when not entered: |
michael-s-molina
added
the
v5.0
(cherry picked from commit 8984f88)
(cherry picked from commit 8984f88)
SUMMARY
Small adjustment to add an ellipsis and width to the legend based on the margin so that the legend does not overlap the chart when on the sides. Currently you have to manually adjust the margin to make sure that it all fits. There are still some cases in very small margins where there is still some overlap, and manual adjustment to the margins is necessary.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
Screen.Recording.2025-01-31.at.3.01.42.PM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION