You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "VanessaGiannoni (via GitHub)" <gi...@apache.org> on 2026/01/23 20:03:21 UTC
[PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
VanessaGiannoni opened a new pull request, #37405:
URL: https://github.com/apache/superset/pull/37405
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes label positioning and readability issues for negative values in stacked bar charts. Previously, labels on negative stacked bars were positioned outside the chart area making them hard to understand in relation to their data segments.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
#### Test only with NEGATIVE values
1. Create a stacked bar chart with mixed positive and negative values:
2. Go to http://localhost:8088/sqllab and paste this script
```SQL
WITH chart_data AS (
SELECT *
FROM (VALUES
('Bar 1', 'Segment A', -10),
('Bar 1', 'Segment B', -5),
('Bar 2', 'Segment A', -7),
('Bar 2', 'Segment B', -3),
('Bar 3', 'Segment A', -12),
('Bar 3', 'Segment B', -6)
) AS t(bar, segment, value)
)
SELECT *
FROM chart_data;
```
3. Then press the "Create chart" button
<img width="190" height="103" alt="image" src="https://github.com/user-attachments/assets/8fbacf08-bc26-467a-833a-b2f81894854d" />
4. Configure:
- Chart Type: Time-series Bar Chart
- X-axis: column1
- Metrics: SUM(column3)
- Customize tab:
- Show Value: ✓ Enable
- Stacked Style: Stack
5. Check the labels are positioned inside the bar at the very edge
6. Change the color scheme and make sure the label changes its color to keep the color ratio according to AA standard
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "michaelmorera92 (via GitHub)" <gi...@apache.org>.
michaelmorera92 commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3863110923
This ticket was tested. The application is working as expected.
# Scenarios tested
Verify the bar chart and values are correctly displayed when the values are negative.
| Variant | Status |
| -------- | -------- |
| Negative values | :white_check_mark: |
| Positive values | :white_check_mark: |
| Vertical view | :white_check_mark: |
| Horizontal view | :white_check_mark: |
| Stacked Style: stack | :white_check_mark: |
| Stacked Style: none | :white_check_mark: |
## Steps
1. Create a stacked bar chart with mixed positive and negative values:
2. Go to http://<ephemeral_or_showtime>/sqllab and paste the "SQL Lab Query" (provided later on this comment) script for ALL negative values
3. Press the "Create chart" button
4. Configure:
- Chart Type: Time-series Bar Chart
- X-axis: column1
- Metrics: SUM(column3)
- Customize tab:
- Show Value: ✓ Enable
- Stacked Style: Stack
## Expected result
After the step 4 (clicking the update chart button):
- Check the labels are positioned inside the bar at the very edge.
- Check the color scheme changed and make sure the label changes its color to keep the color ratio according to AA standard
## SQL Lab Query
> WITH chart_data AS (
> SELECT *
> FROM (VALUES
> ('Bar 1', 'Segment A', -10),
> ('Bar 1', 'Segment B', -5),
> ('Bar 2', 'Segment A', -7),
> ('Bar 2', 'Segment B', -3),
> ('Bar 3', 'Segment A', -12),
> ('Bar 3', 'Segment B', -6)
> ) AS t(bar, segment, value)
> )
> SELECT *
> FROM chart_data;
## Evidence
### Negative, fixed and not fixed comparison.
### Color changes also applied correctly to positive values.
<img width="1914" height="999" alt="Screenshot 2026-02-06 at 5 51 24 PM" src="https://github.com/user-attachments/assets/e4809709-5dcb-49fc-8aae-c8478631d489" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3792147036
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "VanessaGiannoni (via GitHub)" <gi...@apache.org>.
VanessaGiannoni commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2746594633
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -45,7 +45,7 @@ describe('transformSeries', () => {
const result = transformSeries(series, mockColorScale, 'test-key', opts);
- expect((result as any)?.itemStyle.color).toBe('color-for-test-key-1');
+ expect((result as any)?.itemStyle.color).toBeDefined();
});
Review Comment:
Good point, I changed to be like this now:
- when `timeShiftColor` is enabled, it uses the `colorScaleKey`, and when disabled, it uses the `seriesKey`
For the CI issues, it was a type problem that I've fixed. I just followed the previous mock typing to use the `unknown` casting because `Partial` didn't work as I expected.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3792163209
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 1ec318a112737e07fb2468628d45b38818d2e278 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6973d40a271ed7000882d535 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-37405--superset-docs-preview.netlify.app](https://deploy-preview-37405--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37405--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2753408228
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
Review Comment:
Just a nit, but since it adjusts all the label positions not just for negative values we should probably rename this function right?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3862764907
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21767559398) for [54136cc](https://github.com/apache/superset/commit/54136cc)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3869228761
Needs a quick rebase and i think we are good to go. @VanessaGiannoni
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3862855617
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/21767559398) for [54136cc](https://github.com/apache/superset/commit/54136cc)
• **Environment:** http://52.39.158.249:8080 (admin/admin)
• **Lifetime:** 72h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #37405:
URL: https://github.com/apache/superset/pull/37405
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3834063867
Maybe a quick rebase would also help.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2745466866
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -45,7 +45,7 @@ describe('transformSeries', () => {
const result = transformSeries(series, mockColorScale, 'test-key', opts);
- expect((result as any)?.itemStyle.color).toBe('color-for-test-key-1');
+ expect((result as any)?.itemStyle.color).toBeDefined();
});
Review Comment:
Wondering if it is a better way to check that it is actually using the colorScaleKey or the seriesKey?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "VanessaGiannoni (via GitHub)" <gi...@apache.org>.
VanessaGiannoni commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2746594633
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -45,7 +45,7 @@ describe('transformSeries', () => {
const result = transformSeries(series, mockColorScale, 'test-key', opts);
- expect((result as any)?.itemStyle.color).toBe('color-for-test-key-1');
+ expect((result as any)?.itemStyle.color).toBeDefined();
});
Review Comment:
Good point, I changed to be like this now:
- when `timeShiftColor` is enabled, it uses the `colorScaleKey`, and when disabled, it uses the `seriesKey`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "VanessaGiannoni (via GitHub)" <gi...@apache.org>.
VanessaGiannoni commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2746620335
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -361,8 +375,11 @@ export function transformSeries(
symbolSize: markerSize,
label: {
show: !!showValue,
- position: isHorizontal ? 'right' : 'top',
- color: theme?.colorText,
+ position: stack ? 'inside' : isHorizontal ? 'right' : 'top',
+ color:
+ stack || seriesType === 'bar'
+ ? getContrastingColor(String(itemStyle.color))
Review Comment:
**Regarding label position override:**
ECharts explicitly supports data-level label positions overriding series-level positions. This is documented ECharts behavior - when both are set, the data-level position takes precedence. Our tests confirm this works correctly, and manual testing shows labels appear in the expected positions (insideTop/insideBottom/etc.).
**Regarding color type safety:**
The `itemStyle.color` is guaranteed to be a string at this point in the code flow because:
It's set directly from `colorScale(colorKey, sliceId)` which returns a string color
The `String()` wrapper provides additional safety even if somehow a non-string were passed
Our comprehensive tests verify the color flow works correctly with valid hex colors
The current implementation follows the existing codebase patterns and has been thoroughly tested. Adding extra guards would add unnecessary complexity without solving actual problems, violating the YAGNI principle we're following in this codebase.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "VanessaGiannoni (via GitHub)" <gi...@apache.org>.
VanessaGiannoni commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2746607394
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -28,10 +28,12 @@ import {
import transformProps from '../../src/Timeseries/transformProps';
import { EchartsTimeseriesChartProps } from '../../src/types';
-// Mock the colorScale function
-const mockColorScale = jest.fn(
- (key: string, sliceId?: number) => `color-for-${key}-${sliceId}`,
-) as unknown as CategoricalColorScale;
+// Mock the colorScale function to return different colors based on key
+const mockColorScale = jest.fn((key: string) => {
+ if (key === 'test-key') return '#1f77b4'; // blue
+ if (key === 'series-key') return '#ff7f0e'; // orange
+ return '#2ca02c'; // green for any other key
+}) as unknown as CategoricalColorScale;
Review Comment:
I left the `unknown` casting as it was already used because we aren't using all the `CategoricalColorScale` props for this mock, and using `Partial` didn't work as expected
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3824331220
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2746596987
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -361,8 +375,11 @@ export function transformSeries(
symbolSize: markerSize,
label: {
show: !!showValue,
- position: isHorizontal ? 'right' : 'top',
- color: theme?.colorText,
+ position: stack ? 'inside' : isHorizontal ? 'right' : 'top',
+ color:
+ stack || seriesType === 'bar'
+ ? getContrastingColor(String(itemStyle.color))
Review Comment:
**Suggestion:** The series-level `label.position` is set to a generic `'inside'` when `stack` is true, which will override per-data label positions (such as 'insideLeft'/'insideRight') created by `transformNegativeLabelsPosition`. Also the `color` expression calls `getContrastingColor(String(itemStyle.color))` without guarding against `itemStyle.color` being undefined or a non-string, which can produce invalid inputs. Change the series-level `position` to allow data-level overrides when stacked and guard/resolves the item color before calling `getContrastingColor`, falling back to `theme?.colorText`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Stacked bar negative labels mispositioned visually.
- ⚠️ Timeseries ECharts rendering for stacked bars affected.
- ⚠️ Accessibility/readability of negative values impaired.
- ⚠️ Label color may be unexpected if color missing.
```
</details>
```suggestion
// Allow data-level label.position (e.g. 'insideLeft'/'insideRight') to take precedence when stacked
position: stack ? undefined : isHorizontal ? 'right' : 'top',
color:
stack || seriesType === 'bar'
? (itemStyle.color ? getContrastingColor(String(itemStyle.color)) : theme?.colorText)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Produce a stacked bar timeseries with negative values in Superset (Timeseries ECharts
pipeline uses this file). The per-data transform is defined at
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts in
transformNegativeLabelsPosition (lines ~141-174 in the PR hunk). That function returns
per-data label objects with position 'insideLeft' or 'insideRight' for negative/positive
values.
2. When the chart renderer builds series options it calls transformSeries in the same file
(series construction begins in the hunk around line 329). For a bar series with stacking
enabled, transformSeries will call transformNegativeLabelsPosition(series, isHorizontal)
to produce data entries with label positions (see transformNegativeLabelsPosition at lines
~141-174).
3. Later in transformSeries the series-level label configuration is set to position: stack
? 'inside' : ... (exact lines 376-378). ECharts applies a series-level label.position over
data-level label.position, so this assignment overrides the per-data positions produced in
step 2.
4. Render the chart in the browser and inspect the generated ECharts option (developer
tools). You will observe that data-level label positions ('insideLeft'/'insideRight') are
not taking effect and all labels use the series-level 'inside' placement, causing
negative-segment labels to be mispositioned relative to their bar edges (visual regression
the PR intended to fix).
5. (Color fallback observation, lower-likelihood) The series-level color expression calls
getContrastingColor(String(itemStyle.color)) at lines 379-381. If itemStyle.color is
absent or not a plain color string (for example in a contrived test where colorScale
returns undefined or a non-string object), String(itemStyle.color) yields 'undefined' or
'[object Object]'. This produces an unexpected input to getContrastingColor and may yield
an invalid contrast color. Reproducing this requires forcing itemStyle.color to be
undefined (e.g., mocking colorScale to return undefined) and observing label color output.
This is a realistic but uncommon scenario.
Note: The label-position problem is a behavior vs. intention mismatch —
transformNegativeLabelsPosition sets data-level positions, but the later series-level
position unconditionally overrides them.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
**Line:** 378:381
**Comment:**
*Logic Error: The series-level `label.position` is set to a generic `'inside'` when `stack` is true, which will override per-data label positions (such as 'insideLeft'/'insideRight') created by `transformNegativeLabelsPosition`. Also the `color` expression calls `getContrastingColor(String(itemStyle.color))` without guarding against `itemStyle.color` being undefined or a non-string, which can produce invalid inputs. Change the series-level `position` to allow data-level overrides when stacked and guard/resolves the item color before calling `getContrastingColor`, falling back to `theme?.colorText`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -28,10 +28,12 @@ import {
import transformProps from '../../src/Timeseries/transformProps';
import { EchartsTimeseriesChartProps } from '../../src/types';
-// Mock the colorScale function
-const mockColorScale = jest.fn(
- (key: string, sliceId?: number) => `color-for-${key}-${sliceId}`,
-) as unknown as CategoricalColorScale;
+// Mock the colorScale function to return different colors based on key
+const mockColorScale = jest.fn((key: string) => {
+ if (key === 'test-key') return '#1f77b4'; // blue
+ if (key === 'series-key') return '#ff7f0e'; // orange
+ return '#2ca02c'; // green for any other key
+}) as unknown as CategoricalColorScale;
Review Comment:
**Suggestion:** The test's `mockColorScale` Jest mock is declared to accept a single parameter but the production code calls the color scale with two arguments (key and namespace/sliceId). Typing the mock to accept the optional second parameter matches actual usage and avoids type/intent confusion; replace the loose "unknown as" cast with a direct cast to `CategoricalColorScale`. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unit test type-safety masked by double-cast.
- ⚠️ Developers may be misled about mock signature.
```
</details>
```suggestion
const mockColorScale = jest.fn((key: string, namespace?: number) => {
if (key === 'test-key') return '#1f77b4'; // blue
if (key === 'series-key') return '#ff7f0e'; // orange
return '#2ca02c'; // green for any other key
}) as CategoricalColorScale;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the unit test file at
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts and
locate the mock definition at lines 31-36. The mock is declared as:
// Mock the colorScale function to return different colors based on key
const mockColorScale = jest.fn((key: string) => { ... }) as unknown as
CategoricalColorScale;
2. Observe transformSeries in
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts (function
starting at line 179). Inside that function the production code calls the colorScale with
two arguments for itemStyle.color:
color: timeShiftColor ? colorScale(colorScaleKey, sliceId) : colorScale(seriesKey ||
forecastSeries.name, sliceId)
3. The test has assertions that expect two-argument calls (e.g.
expect(mockColorScale).toHaveBeenCalledWith('test-key', 1) added at lines ~50-51 and
~63-64 in transformers.test.ts). At runtime Jest accepts any arguments and the test will
pass because the mock implementation ignores typings.
4. If you remove the current double-cast hack ("as unknown as CategoricalColorScale") and
instead cast the single-argument mock directly to CategoricalColorScale without adding the
optional second parameter, TypeScript will raise a compilation/type error when running the
test typecheck (yarn test / ts-jest or tsc). The concrete reproduction is:
- Change the mock cast to "as CategoricalColorScale" but keep the single-arg function
(file: transformers.test.ts, line 36).
- Run the TypeScript compile step used by tests (jest with ts-jest or tsc).
- Observe a type error complaining that "(key: string) => string" is not assignable to
CategoricalColorScale because the second (namespace/sliceId) parameter is missing.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
**Line:** 32:36
**Comment:**
*Type Error: The test's `mockColorScale` Jest mock is declared to accept a single parameter but the production code calls the color scale with two arguments (key and namespace/sliceId). Typing the mock to accept the optional second parameter matches actual usage and avoids type/intent confusion; replace the loose "unknown as" cast with a direct cast to `CategoricalColorScale`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3886256129
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "VanessaGiannoni (via GitHub)" <gi...@apache.org>.
VanessaGiannoni commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2794627721
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
Review Comment:
@alexandrusoare thanks for the catch! It makes sense to rename it. Now is more accurate
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3819727308
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(charts): improve negative stacked bar label positioning and accessibility [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37405:
URL: https://github.com/apache/superset/pull/37405#issuecomment-3822828153
<!-- Bito Reply -->
To check if the code is using colorScaleKey or seriesKey, consider modifying the mock to capture calls and assert on the parameters. This ensures the color logic depends on the key.
**superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts**
```
const mockColorScale = jest.fn((key: string, sliceId?: number) => `color-for-${key}-${sliceId}`);
// In test:
expect(mockColorScale).toHaveBeenCalledWith(expectedKey, expectedSliceId);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org