EnxDev commented on PR #38695:
URL: https://github.com/apache/superset/pull/38695#issuecomment-4901219528
## EnxDev's Review Agent โ apache/superset#38695 ยท HEAD b8fc0d1
**comment** โ feature is well-scoped and tested, but two behavioral items
and red CI stand between it and merge.
Feature PR: adds a `Label Position` control
(Auto/Top/Inside/Bottom/Left/Right) to bar/mixed-timeseries value labels,
threaded through `transformSeries`. Reviewed against #27553 and the prior
maintainer feedback (@rusackas: rebase, strip reformatting, reconsider Mixed
default). The reformatting *was* stripped and the Mixed default is now `auto` โ
both addressed. Two things remain.
### ๐ด Functional
- **`Timeseries/transformers.ts:419-426`** ยท _High_ โ `auto` for
**horizontal** orientation now resolves to `inside`; it was `right`
(`isHorizontal ? 'right' : 'top'`). Every existing horizontal bar chart with
`Show Values` and no saved `labelPosition` (i.e. all of them) shifts labels
from the bar's right to inside. The PR says "no breaking change," but this *is*
a change, and #27553 is about **vertical stacked** bars โ it doesn't ask for
the horizontal default to move. Either keep `right` for horizontal `auto`, or
make this a deliberate, documented default change. **regression test:**
horizontal bar formData with `labelPosition` unset (or `'auto'`) โ assert
`series.label.position === 'right'` (the preserved default).
- **`Timeseries/transformers.ts:381-382` + `:159-166`** ยท _Medium_ โ On
unstacked bar charts with negative values, `transformNegativeLabelsPosition`
writes per-point `label.position: 'outside'`, which overrides the series-level
position. So a user who picks `Inside` still sees negative bars labeled
`outside` โ the new control is silently ignored for negatives. Make the helper
respect an explicit non-`auto` `labelPosition` (fall back to `outside` only
when position is `auto`/unset). **regression test:** unstacked bar, data with a
negative value, `labelPosition:'inside'` โ assert the negative point's
`label.position === 'inside'`.
### ๐ก Should-fix
- **CI / mergeability** โ `pre-commit (current)` is **red** at `b8fc0d1`
(exit 1) and GitHub can't compute a clean merge (conflict noted earlier by
@rusackas). Rebase on `master` and run `pre-commit run --all-files` locally
until green โ this gates merge regardless of the review.
- **`Timeseries/transformers.ts:427`** โ `overflow: 'truncate'` is added
unconditionally to *every* series (line/area/scatter/bar), not just bars.
Harmless for unbounded positions (top/right/outside), but it will clip `inside`
labels on any series type once width-bounded. Confirm that's intended, or scope
it to bar series.
- **Coverage** โ codecov patch is 75%; the 2 uncovered lines are the
`visibility` callbacks in `controls.tsx` and
`MixedTimeseries/controlPanel.tsx`. Minor, but the show/hide logic of the new
control is untested.
### ๐ต Nits
- **`Timeseries/types.ts:54-64`** โ `TimeseriesLabelPosition` exposes
`insideTop/insideBottom/insideLeft/insideRight`, but the UI offers only 6 of
the 11 values. Fine if the extras are API-only; otherwise trim the type or add
the choices.
### ๐ Praise
- `test/MixedTimeseries/transformProps.test.ts:429` โ the `test.each`
covering per-query A/B positions (incl. `undefined`) is exactly the regression
guard the earlier review asked for.
---
### Review-thread status (requested: resolve addressed conversations)
I can't resolve/close threads myself โ this agent is **read + comment
only**; resolving a review thread is a GraphQL mutation and is blocked at the
permission layer. Here's the addressed/open split so they can be resolved by
hand (or with Bito's `/resolve`):
**Addressed by the current diff โ safe to resolve:**
- Mixed control missing `auto` / default `top` โ now `['auto', โฆ]`,
`default: 'auto'` (codeant `r2960736688`, bito `r2961266288`).
- "relative to the bar segment" wording โ now "relative to the data point"
(Copilot on `controls.tsx` & `controlPanel.tsx`).
- Test cast `series as any[]` โ now `seriesType[]` (Copilot, bito).
- Missing A/B `label.position` assertions in Mixed tests โ added (Copilot).
- `show_valueB` dead-code visibility check โ now `show_value` only (Copilot).
- Left/right grid-padding changes and `() => { }` no-op reformatting โ
reverted, no longer in the diff (codeant, Copilot).
- Query A/B max-bound-vs-axis suggestions โ target code is no longer touched
by this PR (codeant `r...311795`, `r...311820`) โ outdated.
**Still open โ do not resolve:**
- Negative-value labels ignore the chosen position (codeant `r...311829` /
`r3531220603`) โ ๐ด #2 above.
- `auto` horizontal `right`โ`inside` behavior change (Copilot on
`transformers.ts`) โ ๐ด #1 above.
<!-- enxdev-review-agent:b8fc0d1 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD b8fc0d1._
--
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]
