rusackas commented on PR #39137:
URL: https://github.com/apache/superset/pull/39137#issuecomment-4436937887
AI assisted review points below... I think most of this is minor polish, but
I'd agree with at least addressing the safeLabelWidth naming redundancy and
consider adding a comment explaining the 0.62 constant added.
1) Canvas created but never cleaned up. A new <canvas> element is created
every time transformProps is called, but it's never appended to the DOM and
there's no explicit cleanup. In modern browsers this is generally fine since
it'll be GC'd, but it's slightly wasteful — a module-level singleton canvas
could be reused.
2) Font matching may not be exact. The font is set to ${theme.fontSizeSM}px
${theme.fontFamily}, but ECharts internally may render labels slightly
differently (e.g., accounting for DPR, letter spacing, or font weight). The
measured width should be close enough in practice, but labels with tight
padding could still get slightly clipped on some systems.
3) safeLabelWidth is just maxCategoryLabelWidth renamed. The variable
safeLabelWidth is assigned directly from maxCategoryLabelWidth with no further
transformation, which adds no value and slightly obscures the intent. This
could be a leftover from a refactor.
4) No padding budget / cap. If a label is extremely long (e.g., 500px), the
left grid will just grow unboundedly. A reasonable max width or truncation
strategy might be worth considering — though that might be a separate issue.
Partial test coverage. Codecov flags 3 uncovered lines. Given the typeof
document !== 'undefined' branch, it would be good to ensure the SSR/fallback
path is tested.
--
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]
