sadpandajoe commented on code in PR #44208:
URL: https://github.com/apache/superset/pull/44208#discussion_r4150906589
##########
superset-frontend/packages/superset-ui-core/src/components/TableCollection/utils.tsx:
##########
@@ -84,35 +90,39 @@ function getSortingInfo<T extends object>(
}
export function mapColumns<T extends object>(
- columns: EnhancedColumnInstance<T>[],
- headerGroups: EnhancedHeaderGroup<T>[],
+ columns: ListViewColumn<T>[],
+ headerGroups: HeaderGroup<T>[],
columnsForWrapText?: string[],
) {
return columns.map(column => {
- const { isSorted, isSortedDesc } = getSortingInfo(headerGroups, column.id);
+ const id = column.id ?? '';
Review Comment:
The Action Log shared-sort-state regression would still pass the current
TableCollection tests, which only supply columns with IDs. Could we add a case
with two raw id-less columns, click one header, and assert that only that
header acquires sorted `aria-sort` state?
##########
superset-frontend/packages/superset-ui-core/src/components/TableCollection/utils.tsx:
##########
@@ -49,23 +40,38 @@ const COLUMN_SIZE_MAP: Record<TableSize, number> = {
xxl: 200,
};
-type EnhancedColumnInstance<T extends object = any> = RTColumnInstance<T> &
- Partial<UseSortByColumnOptions<T>> &
- Partial<UseSortByColumnProps<T>> &
- Partial<UseResizeColumnsColumnOptions<T>> &
- Partial<UseResizeColumnsColumnProps<T>> & {
- hidden?: boolean;
- size?: keyof typeof COLUMN_SIZE_MAP;
- className?: string;
- };
+// Mirrors react-table's `Renderer<Props>` (a component, a render function,
+// or a static node) loosely enough that both a hand-authored column config
+// and a `ColumnInstance<T>` produced by `useTable()` satisfy it, so callers
+// holding either shape can pass it without casting.
+type ColumnRenderer =
+ | ReactNode
+ | ComponentType<any>
+ | ((props: any) => ReactNode);
-type EnhancedHeaderGroup<T extends object = any> = RTHeaderGroup<T> & {
- isSorted?: boolean;
- isSortedDesc?: boolean;
-};
+// The `columns` prop is typically the raw column config a caller authors,
+// though the `ColumnInstance<T>` objects react-table builds from that config
+// are structurally accepted too. This is intentionally its own interface
+// rather than react-table's `Column<T>`: that type requires each column's
+// `accessor` to be either a specific `keyof T` literal or an accessor
+// function, but every column config in this codebase writes `accessor` as
+// a plain (TypeScript-widened) string, which satisfies neither — matching
+// react-table's stricter modeling here would mean annotating every column
+// array across ~20 call sites, not a change this shim should make
+// unilaterally.
+export interface ListViewColumn<T extends object = any> {
+ id?: string;
+ Header?: ColumnRenderer;
+ accessor?: keyof T | string | ((row: T) => unknown);
Review Comment:
Valid react-table accessors that use the row index or sub-row context now
fail to type-check when passed to ListView: `Accessor<T>` requires those
arguments, but this type only permits a one-argument function. Could we
preserve react-table’s full accessor signature here?
##########
superset-frontend/packages/superset-ui-core/src/components/TableCollection/utils.tsx:
##########
@@ -121,7 +131,8 @@ export function mapColumns<T extends object>(
column,
});
}
- return val as ReactNode;
+ // A static `Cell` node renders as-is, as react-table itself would.
+ return Cell ?? (val as ReactNode);
Review Comment:
A static `Cell` would crash again if this branch regressed to calling every
truthy renderer, but the current TableCollection tests only exercise generated
default cells. Could we add a case with `Cell: <span>Static</span>` alongside a
column without `Cell`, asserting that the static content and the ordinary data
value both render?
--
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]
