aminghadersohi commented on code in PR #44321:
URL: https://github.com/apache/superset/pull/44321#discussion_r4021666736
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gantt/transformProps.ts:
##########
@@ -453,7 +471,7 @@ export default function transformProps(chartProps:
EchartsGanttChartProps) {
grid: {
...defaultGrid,
...padding,
- left: (padding.left || 0) + maxCategoryLabelWidth + 10,
+ left: (padding.left || 0) + Math.ceil(maxCategoryLabelWidth) + 10,
},
Review Comment:
Validated this against a controlled A/B in a real browser rather than by
inspection. Summary: **the mechanism is real but pre-existing, this PR does not
change it, and the stated consequence ("Gantt bars disappear") does not
reproduce.** No change made here.
**Provenance.** `git log -L 471,475` shows the unbounded reservation on this
line was introduced by #39137 (`8e69572`):
```
- left: (padding.left || 0) + maxCategoryLabelWidth + 10, # 8e69572
(#39137)
+ left: (padding.left || 0) + Math.ceil(maxCategoryLabelWidth) + 10,
# this PR
```
This PR's only edit to the flagged range is wrapping the existing value in
`Math.ceil()`, which can raise `grid.left` by **at most 1px** (0px for the case
below, since the widest label measures an integral 493px).
**Measured A/B.** Real Chromium, real webfonts, real ECharts with real Gantt
bars, 6 category labels (widest ≈493px), plot rect read from
`getModel().getComponent('grid').coordinateSystem.getRect()` and bar presence
counted from canvas pixels:
| container | variant | `grid.left` | plot.x | plot.width | bar pixels |
bars visible |
|---|---|---|---|---|---|---|
| 497px | pre-#39137 (no reservation) | unset | 74.5 | 402.4 | 9246 | yes |
| 497px | parent of this PR | 503 | 477 | **17.1** | 456 | **yes** |
| 497px | **this PR (`87bab7a`)** | 503 | 477 | **17.1** | 456 | **yes** |
| 700px | parent / this PR | 503 / 503 | 503 | 177 / 177 | 4103 / 4103 | yes
|
| 1100px | parent / this PR | 503 / 503 | 503 | 577 / 577 | 13223 / 13223 |
yes |
Two things follow:
1. **This PR is a no-op for this concern.** Parent and PR head are
byte-identical on every metric at every width — same `grid.left`, same plot
rect, same bar pixel count. The squeeze is entirely attributable to #39137's
reservation (plot width 402.4 → 17.1 at 497px).
2. **Bars do not disappear.** ECharts clamps the plot origin (`plot.x` 503 →
477) and still renders, leaving a 17.1px plot area. The bars are severely
squeezed into slivers, but present and non-zero at every width tested. I could
not reproduce a state with zero bars.
The underlying tradeoff is inherent to #39137: reserve space for labels
(bars get squeezed on narrow charts) or don't (labels get clipped — the bug
#39137 fixed). Bounding `grid.left` to a fraction of the container would
address it properly, but that is a responsive-layout change to behavior this PR
does not touch, so it belongs in its own PR rather than being folded into a
label-measurement fix.
Resolving as pre-existing and out of scope. Happy to file the clamp as a
follow-up.
--
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]
