From 7a3d991ea815948cd4e9c4b898311a753a360f78 Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Mon, 16 Jun 2025 11:19:39 +0200 Subject: [PATCH 1/3] refactor(AnalyticalTable): attach scroll methods via `useImperativeHandle` This PR also improves types and adds missing dependencies in dependency arrays. --- .../TableBody/VirtualTableBody.tsx | 6 +-- .../hooks/useTableScrollHandles.ts | 37 ++++++++----------- .../src/components/AnalyticalTable/index.tsx | 10 ++--- .../components/AnalyticalTable/types/index.ts | 13 ++++++- 4 files changed, 35 insertions(+), 31 deletions(-) diff --git a/packages/main/src/components/AnalyticalTable/TableBody/VirtualTableBody.tsx b/packages/main/src/components/AnalyticalTable/TableBody/VirtualTableBody.tsx index ccedb4ae084..ec29d314b7c 100644 --- a/packages/main/src/components/AnalyticalTable/TableBody/VirtualTableBody.tsx +++ b/packages/main/src/components/AnalyticalTable/TableBody/VirtualTableBody.tsx @@ -1,12 +1,12 @@ import type { Virtualizer } from '@tanstack/react-virtual'; import { clsx } from 'clsx'; -import type { MutableRefObject } from 'react'; +import type { MutableRefObject, RefObject } from 'react'; import { useEffect, useMemo, useRef } from 'react'; import type { AnalyticalTablePropTypes, ClassNames, DivWithCustomScrollProp, - ScrollToRefType, + ReactVirtualScrollToMethods, TableInstance, TriggerScrollState, } from '../types/index.js'; @@ -35,7 +35,7 @@ interface VirtualTableBodyProps { subRowsKey: string; scrollContainerRef?: MutableRefObject; triggerScroll?: TriggerScrollState; - scrollToRef: MutableRefObject; + scrollToRef: RefObject; rowVirtualizer: Virtualizer; } diff --git a/packages/main/src/components/AnalyticalTable/hooks/useTableScrollHandles.ts b/packages/main/src/components/AnalyticalTable/hooks/useTableScrollHandles.ts index de87b3b9561..0a3d3cc4ec1 100644 --- a/packages/main/src/components/AnalyticalTable/hooks/useTableScrollHandles.ts +++ b/packages/main/src/components/AnalyticalTable/hooks/useTableScrollHandles.ts @@ -1,8 +1,7 @@ -import type { ScrollToOptions } from '@tanstack/react-virtual'; -import type { MutableRefObject } from 'react'; -import { useEffect, useRef } from 'react'; +import type { ForwardedRef, RefObject } from 'react'; +import { useImperativeHandle, useRef } from 'react'; import type { AnalyticalTableScrollMode } from '../../../enums/index.js'; -import type { AnalyticalTableDomRef } from '../types/index.js'; +import type { AnalyticalTableDomRef, ReactVirtualScrollToMethods, TableInstance } from '../types/index.js'; interface ScrollToMethods { scrollTo: (offset: number, align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode) => void; @@ -17,23 +16,16 @@ interface ScrollToMethods { ) => void; } -interface ReactVirtualScrollToMethods { - scrollToOffset?: (offset: number, options?: ScrollToOptions) => void; - scrollToIndex?: (index: number, options?: ScrollToOptions) => void; - horizontalScrollToOffset?: (offset: number, options?: ScrollToOptions) => void; - horizontalScrollToIndex?: (index: number, options?: ScrollToOptions) => void; -} - -export const useTableScrollHandles = (ref, dispatch) => { - let analyticalTableRef = useRef(null); - if (ref) { - analyticalTableRef = ref; - } +export const useTableScrollHandles = ( + ref: ForwardedRef, + dispatch: TableInstance['dispatch'], +): RefObject => { const scrollToRef = useRef({}); - useEffect(() => { - if (analyticalTableRef.current) { - Object.assign, ScrollToMethods>(analyticalTableRef.current, { + useImperativeHandle(ref, () => { + const atNode = (ref as RefObject)?.current; + if (atNode) { + const scrollMethods: ScrollToMethods = { scrollTo: (offset, align) => { if (typeof scrollToRef.current?.scrollToOffset === 'function') { scrollToRef.current.scrollToOffset(offset, { align }); @@ -74,9 +66,10 @@ export const useTableScrollHandles = (ref, dispatch) => { }); } }, - }); + }; + return Object.assign(atNode, scrollMethods); } - }, []); + }, [dispatch, ref]); - return [analyticalTableRef, scrollToRef]; + return scrollToRef; }; diff --git a/packages/main/src/components/AnalyticalTable/index.tsx b/packages/main/src/components/AnalyticalTable/index.tsx index 158a5d6b24e..646dc3a9a9f 100644 --- a/packages/main/src/components/AnalyticalTable/index.tsx +++ b/packages/main/src/components/AnalyticalTable/index.tsx @@ -307,9 +307,9 @@ const AnalyticalTable = forwardRef 0 || tableState.globalFilter ? noDataTextFiltered : noDataTextI18n); - const [componentRef, updatedRef] = useSyncRef(ref); + const [componentRef, analyticalTableRef] = useSyncRef(ref); //@ts-expect-error: types are compatible - const isRtl = useIsRTL(updatedRef); + const isRtl = useIsRTL(analyticalTableRef); const columnVirtualizer = useVirtualizer({ count: visibleColumnsWidth.length, @@ -340,7 +340,7 @@ const AnalyticalTable = forwardRef { if ( diff --git a/packages/main/src/components/AnalyticalTable/types/index.ts b/packages/main/src/components/AnalyticalTable/types/index.ts index 317097b9996..f6b7c59617b 100644 --- a/packages/main/src/components/AnalyticalTable/types/index.ts +++ b/packages/main/src/components/AnalyticalTable/types/index.ts @@ -101,7 +101,11 @@ export interface TableInstance { disableGlobalFilter?: boolean; disableGroupBy?: boolean; disableSortBy?: boolean; - dispatch?: (action: any) => void; + dispatch?: (action: { + type: string; + payload?: Record | AnalyticalTableState['popInColumns'] | boolean | string; + clientX?: number; + }) => void; expandedDepth?: number; expandedRows?: RowType[]; filteredFlatRows?: RowType[]; @@ -280,6 +284,13 @@ export interface TriggerScrollState { args: [number, Omit?]; } +export interface ReactVirtualScrollToMethods { + scrollToOffset?: (offset: number, options?: ScrollToOptions) => void; + scrollToIndex?: (index: number, options?: ScrollToOptions) => void; + horizontalScrollToOffset?: (offset: number, options?: ScrollToOptions) => void; + horizontalScrollToIndex?: (index: number, options?: ScrollToOptions) => void; +} + interface PopInColumnsState { id: string; column: ColumnType; From aec11e3117b4427639b9e4978c2852d8a1b005a1 Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Mon, 16 Jun 2025 14:36:38 +0200 Subject: [PATCH 2/3] stabilize test --- .../AnalyticalTable/AnalyticalTable.cy.tsx | 65 ++++++++----------- 1 file changed, 28 insertions(+), 37 deletions(-) diff --git a/packages/main/src/components/AnalyticalTable/AnalyticalTable.cy.tsx b/packages/main/src/components/AnalyticalTable/AnalyticalTable.cy.tsx index 761987a8c25..99e167dbc4f 100644 --- a/packages/main/src/components/AnalyticalTable/AnalyticalTable.cy.tsx +++ b/packages/main/src/components/AnalyticalTable/AnalyticalTable.cy.tsx @@ -256,6 +256,16 @@ describe('AnalyticalTable', () => { }); it('autoResize', () => { + function doubleClickResizer(selector: string, columnName: string, outerWidth: number) { + cy.get(selector) + .realHover() + .should('have.css', 'background-color', cssVarToRgb('--sapContent_DragAndDropActiveColor')) + .dblclick() + // fallback + .realClick({ clickCount: 2 }); + cy.get(`[data-column-id="${columnName}"]`).invoke('outerWidth').should('equal', outerWidth); + } + let resizeColumns = columns.map((el) => { return { ...el, autoResizable: true }; }); @@ -277,26 +287,19 @@ describe('AnalyticalTable', () => { }} />, ); - cy.wait(100); cy.get('[data-component-name="AnalyticalTableResizer"]').eq(0).as('resizer1'); cy.get('[data-component-name="AnalyticalTableResizer"]').eq(1).as('resizer2'); - cy.get('@resizer2').should('be.visible').dblclick(); - cy.get('[data-column-id="age"]').invoke('outerWidth').should('equal', 476); - cy.get('@resizer1').should('be.visible').dblclick(); - cy.get('[data-column-id="name"]').invoke('outerWidth').should('equal', 476); - - cy.get('@resize').should('have.callCount', 2); + doubleClickResizer('@resizer2', 'age', 476); + doubleClickResizer('@resizer1', 'name', 476); + // doubled call count because of fallback + cy.get('@resize').should('have.callCount', 4); cy.mount(); - cy.wait(100); - cy.get('@resizer2').should('be.visible').dblclick(); - cy.get('[data-column-id="age"]').invoke('outerWidth').should('equal', 60); - cy.get('@resizer1').should('be.visible').dblclick(); - cy.get('[data-column-id="name"]').invoke('outerWidth').should('equal', 129); - - cy.get('@resize').should('have.callCount', 4); + doubleClickResizer('@resizer2', 'age', 60); + doubleClickResizer('@resizer1', 'name', 129); + cy.get('@resize').should('have.callCount', 8); dataFixed = generateMoreData(200); @@ -319,10 +322,8 @@ describe('AnalyticalTable', () => { ); cy.get('[data-component-name="AnalyticalTableBody"]').scrollTo('bottom'); - cy.get('@resizer1').should('be.visible').dblclick(); - cy.get('[data-column-id="name"]').invoke('outerWidth').should('equal', 93); - - cy.get('@resize').should('have.callCount', 5); + doubleClickResizer('@resizer1', 'name', 93); + cy.get('@resize').should('have.callCount', 10); resizeColumns = columns.map((el) => { return { ...el, autoResizable: false }; @@ -330,12 +331,10 @@ describe('AnalyticalTable', () => { cy.mount(); cy.wait(100); - cy.get('@resizer2').should('be.visible').dblclick(); - cy.get('[data-column-id="age"]').invoke('outerWidth').should('equal', 472.75); - cy.get('@resizer1').should('be.visible').dblclick(); - cy.get('[data-column-id="name"]').invoke('outerWidth').should('equal', 472.75); + doubleClickResizer('@resizer2', 'age', 472.75); + doubleClickResizer('@resizer1', 'name', 472.75); - cy.get('@resize').should('have.callCount', 5); + cy.get('@resize').should('have.callCount', 10); const dataSub = data.map((el, i) => { if (i === 2) return { ...el, name: 'Longer Name Too' }; @@ -358,25 +357,17 @@ describe('AnalyticalTable', () => { onAutoResize={resizeSpy} />, ); - cy.wait(100); - cy.get('@resizer2').should('be.visible').dblclick(); - cy.get('[data-column-id="age"]').invoke('outerWidth').should('equal', 60); - cy.get('@resizer1').should('be.visible').dblclick(); - cy.get('[data-column-id="name"]').invoke('outerWidth').should('equal', 165); - - cy.get('@resize').should('have.callCount', 7); + doubleClickResizer('@resizer2', 'age', 60); + doubleClickResizer('@resizer1', 'name', 165); + cy.get('@resize').should('have.callCount', 14); const dataResizeTree = [...dataTree]; dataResizeTree[0].subRows[0].name = 'Longer Name To Resize Here'; cy.mount(); - cy.wait(100); - cy.get('@resizer1').should('be.visible').dblclick(); - cy.get('[data-column-id="name"]').invoke('outerWidth').should('equal', 169); + doubleClickResizer('@resizer1', 'name', 169); cy.get('[aria-rowindex="1"] > [aria-colindex="1"] > [title="Expand Node"] > [ui5-button]').click(); - cy.get('@resizer1').should('be.visible').dblclick(); - cy.get('[data-column-id="name"]').invoke('outerWidth').should('equal', 251); - - cy.get('@resize').should('have.callCount', 9); + doubleClickResizer('@resizer1', 'name', 251); + cy.get('@resize').should('have.callCount', 18); }); it('scrollTo', () => { From 1fdd6084f5abdf6e8e174c0aec76c0dc1dd3e6e1 Mon Sep 17 00:00:00 2001 From: Lukas Harbarth Date: Tue, 17 Jun 2025 11:12:37 +0200 Subject: [PATCH 3/3] replace `useImperativeHandle` with cb ref --- ...ableScrollHandles.ts => useScrollToRef.ts} | 64 +++++++++---------- .../src/components/AnalyticalTable/index.tsx | 9 ++- 2 files changed, 34 insertions(+), 39 deletions(-) rename packages/main/src/components/AnalyticalTable/hooks/{useTableScrollHandles.ts => useScrollToRef.ts} (51%) diff --git a/packages/main/src/components/AnalyticalTable/hooks/useTableScrollHandles.ts b/packages/main/src/components/AnalyticalTable/hooks/useScrollToRef.ts similarity index 51% rename from packages/main/src/components/AnalyticalTable/hooks/useTableScrollHandles.ts rename to packages/main/src/components/AnalyticalTable/hooks/useScrollToRef.ts index 0a3d3cc4ec1..8b45f488f0b 100644 --- a/packages/main/src/components/AnalyticalTable/hooks/useTableScrollHandles.ts +++ b/packages/main/src/components/AnalyticalTable/hooks/useScrollToRef.ts @@ -1,32 +1,20 @@ -import type { ForwardedRef, RefObject } from 'react'; -import { useImperativeHandle, useRef } from 'react'; +import type { RefCallback, RefObject } from 'react'; +import { useCallback, useRef } from 'react'; import type { AnalyticalTableScrollMode } from '../../../enums/index.js'; -import type { AnalyticalTableDomRef, ReactVirtualScrollToMethods, TableInstance } from '../types/index.js'; +import type { AnalyticalTableDomRef, ScrollToRefType, TableInstance } from '../types/index.js'; -interface ScrollToMethods { - scrollTo: (offset: number, align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode) => void; - scrollToItem: (index: number, align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode) => void; - horizontalScrollTo: ( - offset: number, - align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode, - ) => void; - horizontalScrollToItem: ( - index: number, - align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode, - ) => void; -} - -export const useTableScrollHandles = ( - ref: ForwardedRef, +export function useScrollToRef( + componentRef: (node: AnalyticalTableDomRef) => void, dispatch: TableInstance['dispatch'], -): RefObject => { - const scrollToRef = useRef({}); +): [RefCallback, RefObject] { + const scrollToRef = useRef(null); + + const cbRef: RefCallback = useCallback( + (node) => { + if (!node) return; - useImperativeHandle(ref, () => { - const atNode = (ref as RefObject)?.current; - if (atNode) { - const scrollMethods: ScrollToMethods = { - scrollTo: (offset, align) => { + const extendedNode = Object.assign(node, { + scrollTo: (offset: number, align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode) => { if (typeof scrollToRef.current?.scrollToOffset === 'function') { scrollToRef.current.scrollToOffset(offset, { align }); } else { @@ -36,7 +24,7 @@ export const useTableScrollHandles = ( }); } }, - scrollToItem: (index, align) => { + scrollToItem: (index: number, align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode) => { if (typeof scrollToRef.current?.scrollToIndex === 'function') { scrollToRef.current.scrollToIndex(index, { align }); } else { @@ -46,7 +34,10 @@ export const useTableScrollHandles = ( }); } }, - horizontalScrollTo: (offset, align) => { + horizontalScrollTo: ( + offset: number, + align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode, + ) => { if (typeof scrollToRef.current?.horizontalScrollToOffset === 'function') { scrollToRef.current.horizontalScrollToOffset(offset, { align }); } else { @@ -56,7 +47,10 @@ export const useTableScrollHandles = ( }); } }, - horizontalScrollToItem: (index, align) => { + horizontalScrollToItem: ( + index: number, + align?: AnalyticalTableScrollMode | keyof typeof AnalyticalTableScrollMode, + ) => { if (typeof scrollToRef.current?.horizontalScrollToIndex === 'function') { scrollToRef.current.horizontalScrollToIndex(index, { align }); } else { @@ -66,10 +60,12 @@ export const useTableScrollHandles = ( }); } }, - }; - return Object.assign(atNode, scrollMethods); - } - }, [dispatch, ref]); + }); - return scrollToRef; -}; + componentRef(extendedNode); + }, + [componentRef, dispatch], + ); + + return [cbRef, scrollToRef]; +} diff --git a/packages/main/src/components/AnalyticalTable/index.tsx b/packages/main/src/components/AnalyticalTable/index.tsx index 646dc3a9a9f..40639619fa3 100644 --- a/packages/main/src/components/AnalyticalTable/index.tsx +++ b/packages/main/src/components/AnalyticalTable/index.tsx @@ -70,10 +70,10 @@ import { useResizeColumnsConfig } from './hooks/useResizeColumnsConfig.js'; import { useRowHighlight } from './hooks/useRowHighlight.js'; import { useRowNavigationIndicators } from './hooks/useRowNavigationIndicator.js'; import { useRowSelectionColumn } from './hooks/useRowSelectionColumn.js'; +import { useScrollToRef } from './hooks/useScrollToRef.js'; import { useSelectionChangeCallback } from './hooks/useSelectionChangeCallback.js'; import { useSingleRowStateSelection } from './hooks/useSingleRowStateSelection.js'; import { useStyling } from './hooks/useStyling.js'; -import { useTableScrollHandles } from './hooks/useTableScrollHandles.js'; import { useToggleRowExpand } from './hooks/useToggleRowExpand.js'; import { useVisibleColumnsWidth } from './hooks/useVisibleColumnsWidth.js'; import { VerticalScrollbar } from './scrollbars/VerticalScrollbar.js'; @@ -308,7 +308,8 @@ const AnalyticalTable = forwardRef 0 || tableState.globalFilter ? noDataTextFiltered : noDataTextI18n); const [componentRef, analyticalTableRef] = useSyncRef(ref); - //@ts-expect-error: types are compatible + const [cbRef, scrollToRef] = useScrollToRef(componentRef, dispatch); + // @ts-expect-error: is HTMLElement const isRtl = useIsRTL(analyticalTableRef); const columnVirtualizer = useVirtualizer({ @@ -340,8 +341,6 @@ const AnalyticalTable = forwardRef {header && (