Re: [PR] fix(core): align Drill to detail table headers with scrollable body [superset]

["sadpandajoe (via GitHub)" <gi...@apache.org>]

sadpandajoe commented on code in PR #43472:
URL: https://github.com/apache/superset/pull/43472#discussion_r3991435453


##########
superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx:
##########
@@ -147,6 +149,28 @@ const VirtualTable = <RecordType extends object>(
   const { ref } = useResizeDetector({ onResize });
   const theme = useTheme();
 
+  const cellSize = size === TableSize.Middle ? MIDDLE : SMALL;
+
+  // The header is rendered by antd as a plain `<thead>`, separate from the
+  // react-window `Grid` that renders the body. When the Grid's own vertical
+  // scrollbar appears, it eats into the body's visible column width without
+  // shrinking the header - misaligning the two. Reserve that same space when
+  // sizing columns so both stay in sync (mirrors the fix applied to
+  // `useSticky.tsx` for the non-virtualized table's sticky header).
+  const availableHeight = height || (scroll?.y as number) || 0;
+  // `dataSource` is the full (unpaginated) dataset - when pagination is on,
+  // only a page's worth of rows actually mounts in the Grid at a time, so
+  // that (not the full dataset length) is what determines whether its
+  // vertical scrollbar appears.
+  const pageSize =
+    pagination && typeof pagination === 'object'
+      ? pagination.pageSize
+      : undefined;
+  const rowCount = pageSize ?? dataSource?.length ?? 0;
+  const hasVerticalScroll = rowCount * cellSize > availableHeight;
+  const scrollBarSize = hasVerticalScroll ? getScrollBarSize() : 0;
+  const columnSizingWidth = Math.max(tableWidth - scrollBarSize, 0);

Review Comment:
   rc-table already measures a custom body's native scrollbar, subtracts it from the final header column, and adds the matching header gutter. On non-overlay-scrollbar platforms this pre-shrink removes that width a second time, leaving the header's data columns one scrollbar narrower than the Grid body; could this instead use the `scrollbarSize` passed to `renderVirtualList` to size the body against rc-table's existing header contract?



##########
superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx:
##########
@@ -147,6 +149,28 @@ const VirtualTable = <RecordType extends object>(
   const { ref } = useResizeDetector({ onResize });
   const theme = useTheme();
 
+  const cellSize = size === TableSize.Middle ? MIDDLE : SMALL;
+
+  // The header is rendered by antd as a plain `<thead>`, separate from the
+  // react-window `Grid` that renders the body. When the Grid's own vertical
+  // scrollbar appears, it eats into the body's visible column width without
+  // shrinking the header - misaligning the two. Reserve that same space when
+  // sizing columns so both stay in sync (mirrors the fix applied to
+  // `useSticky.tsx` for the non-virtualized table's sticky header).
+  const availableHeight = height || (scroll?.y as number) || 0;

Review Comment:
   The inherited Ant Table contract allows CSS-string `scroll.y` values, but comparing a row height to `'300px'`, `'50vh'`, or `calc(...)` yields false, so a scrolling Grid gets no header reservation and remains misaligned. Should this derive overflow from the rendered Grid element for non-numeric heights rather than casting the value to a number?



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org