rusackas commented on PR #39287:
URL: https://github.com/apache/superset/pull/39287#issuecomment-4442254116
Thanks for the contribution! The approach is solid — using AG Grid's
`headerTooltip` with `tooltipShowDelay` is exactly the right pattern, and it's
great to see a test file included. A few things to address before merge:
**Bug — stale memoization (`transformProps.ts`):** The new description
lookup reads from `rawDatasource.columns` and `rawDatasource.metrics`, but
`processColumns` is wrapped in `memoizeOne` with a custom `isEqualColumns`
comparator that doesn't account for those fields. If a user updates
column/metric descriptions without changing anything else the comparator
checks, the memoized result will be stale and tooltips will show outdated
values. The fix is to either extend `isEqualColumns` to compare
`rawDatasource.columns`/`rawDatasource.metrics`, or move the description lookup
outside the memoized function.
**Percent-metric key stripping:** The `key.startsWith('%') ? key.slice(1) :
key` logic is clever but fragile without documentation. A short comment
explaining the `%`-prefix convention would help future maintainers.
**Commit history:** The branch has ~69 merge commits (`Merge branch
'apache:master' into master`) that should be squashed before merge. Please
rebase onto master instead.
Otherwise the diff is clean and the feature is a useful parity improvement
with the existing table plugin.
--
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]
