gabotorresruiz commented on code in PR #41976:
URL: https://github.com/apache/superset/pull/41976#discussion_r3589995673
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -81,6 +85,12 @@ const TimeTable = ({
const valueField = row.label || row.metric_name || '';
const cellValues = columnConfigs.reduce<Record<string, ReactNode>>(
(acc, columnConfig) => {
+ const { value, errorMsg } = calculateCellValue(
+ valueField,
+ columnConfig,
+ reversedEntries,
+ );
Review Comment:
This now runs for every column, including `sparklines`, where the result is
thrown away two lines later. Can you move it inside the non spark branch so
spark columns don't pay for a computation they never use?
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -98,9 +108,9 @@ const TimeTable = ({
...acc,
[columnConfig.key]: (
<ValueCell
- valueField={valueField}
+ value={value}
+ errorMsg={errorMsg}
column={columnConfig}
- reversedEntries={reversedEntries}
/>
Review Comment:
I think this prop change breaks column sorting.
`sortNumberWithMixedTypes` in `utils/sortUtils/sortUtils.ts` gets the
rendered cell as the row value and reads `valueField`, column and
`reversedEntries` from its props to compute the comparison values.
With the new props, `reversedEntries` is `undefined`, the guard in the
sorter returns `0` for every pair, and sorting becomes a no op for all value
columns. I verified it on this branch: rendering `TimeTable` with a time
comparison column and clicking the header doesn't reorder rows, while the same
check reorders correctly on master.
The refactor itself is a good idea, but it needs to carry the sorter along:
update `sortNumberWithMixedTypes` to read `cell.props.value` directly for
`ValueCell` columns (keeping the entries path for `Sparkline`), and update the
`sortUtils` tests to the new shape (they currently build mock rows with the old
prop shape by hand, which is why they still pass). That also removes the
duplicated `calculateCellValue` call on every comparison, which is the single
source of truth win this PR is going for. A test that renders `TimeTable` and
asserts row order after a header click would have caught this and would lock
the fix in.
##########
superset-frontend/src/visualizations/TimeTable/components/ValueCell/ValueCell.test.tsx:
##########
@@ -206,15 +208,18 @@ describe('ValueCell', () => {
bounds: [0, 1000] as [number, number],
};
+ const { value, errorMsg } = calculateCellValue(
+ 'sales',
+ columnWithBounds,
+ mockEntries,
+ );
+
const { container } = render(
- <ValueCell
- valueField="sales"
- column={columnWithBounds}
- reversedEntries={mockEntries}
- />,
+ <ValueCell value={value} errorMsg={errorMsg} column={columnWithBounds}
/>,
);
- const span = container.querySelector('span[data-value="300"]');
+ const span = container.querySelector(`span[data-value="${value}"]`);
Review Comment:
Small one: interpolating value into the selector means this assertion can't
fail for any numeric value, it only checks that the prop is echoed into
data-value. Since `ValueCell` is presentational now, I'd go the other way with
these tests: pass literal props directly (value={300}) and assert the literal
span[data-value="300"], rather than deriving inputs through calculateCellValue.
The computed values themselves are already pinned in the valueCalculations
tests, so this file doesn't need to re-derive them.
--
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]
