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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
