EnxDev commented on code in PR #38094:
URL: https://github.com/apache/superset/pull/38094#discussion_r4163020575
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -91,18 +101,26 @@ const TimeTable = ({
entries={entries}
/>
),
+ cellValues: {
+ ...((acc.cellValues as object) || {}),
+ [columnConfig.key]: value,
+ },
};
}
return {
...acc,
[columnConfig.key]: (
<ValueCell
- valueField={valueField}
+ value={value}
column={columnConfig}
- reversedEntries={reversedEntries}
+ errorMsg={errorMsg}
/>
),
+ cellValues: {
+ ...((acc.cellValues as object) || {}),
+ [columnConfig.key]: value,
+ },
Review Comment:
Putting `cellValues` inside the `Record<string, ReactNode>` accumulator is
why we need the `as object` casts here and at 105, and it means the sort values
get spread onto the row together with the rendered cells.
Worth keeping a separate `Record<string, number | null>` in the same loop
and setting `cellValues` explicitly in the row return? That way the types do
the work and the casts go away.
##########
superset-frontend/src/visualizations/TimeTable/utils/sortUtils/sortUtils.ts:
##########
@@ -44,64 +42,23 @@ function compareValues(
}
/**
- * Sorts table rows with mixed data types for react-table.
- *
+ * Sorts table rows with mixed data types for react-table
* @param rowA - First row to compare
* @param rowB - Second row to compare
* @param columnId - Column identifier for sorting
- * @param descending - Whether to sort in descending order
* @returns Numeric comparison result for react-table
- * react-table handles the asc/desc direction flip internally after calling
- * this function, so we only return the raw comparison result.
*/
export function sortNumberWithMixedTypes(
rowA: any,
rowB: any,
columnId: string,
) {
- const cellA = rowA.values?.[columnId];
- const cellB = rowB.values?.[columnId];
-
- // Both ValueCell and Sparkline cells pass React elements here.
- // ValueCell uses { valueField, column, reversedEntries }
- // Sparkline/SparklineCell uses { valueField, column, entries }
- // Normalize to reversedEntries before delegating to calculateCellValue.
- const propsA = cellA?.props as
- | {
- valueField: string;
- column: ColumnConfig;
- reversedEntries?: Entry[];
- entries?: Entry[];
- }
- | undefined;
- const propsB = cellB?.props as
- | {
- valueField: string;
- column: ColumnConfig;
- reversedEntries?: Entry[];
- entries?: Entry[];
- }
- | undefined;
-
- const reversedEntriesA =
- propsA?.reversedEntries ?? propsA?.entries?.slice().reverse();
- const reversedEntriesB =
- propsB?.reversedEntries ?? propsB?.entries?.slice().reverse();
-
- if (!propsA || !propsB || !reversedEntriesA || !reversedEntriesB) {
- return 0;
- }
+ const valueA = rowA.original.cellValues[columnId];
+ const valueB = rowB.original.cellValues[columnId];
Review Comment:
For the rebase: master reads `props.value` off the ValueCell element, and
for spark columns it reruns `calculateCellValue` on `entries.slice().reverse()`
inside every comparison.
I'd keep this version over master's when resolving the conflict, since
reading precomputed values drops that per-comparison work. master's
sortUtils.test.ts mocks the React props shape, so those tests will need to move
to `original.cellValues` too.
##########
superset-frontend/src/visualizations/TimeTable/TimeTable.tsx:
##########
@@ -91,18 +101,26 @@ const TimeTable = ({
entries={entries}
/>
),
+ cellValues: {
+ ...((acc.cellValues as object) || {}),
+ [columnConfig.key]: value,
+ },
Review Comment:
Nothing tests that this map actually gets filled. sortUtils.test.ts builds
`original.cellValues` by hand, so if this wiring breaks (a spark column, say),
every lookup comes back undefined, the comparator returns 0, and the header
click quietly does nothing while all the tests stay green.
Could we add a TimeTable.test.tsx case that renders a few rows, clicks a
time column header and a sparkline header, and checks the row order? That's the
bug from #37892, tested end to end.
--
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]
