Skip to content

TableView reduce renders and onLoadMore #3529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
snowystinger wants to merge 7 commits into from
Closed

TableView reduce renders and onLoadMore #3529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8b32907
TableView reduce renders and onLoadMore
snowystinger Sep 16, 2022
d105405
fix lint
snowystinger Sep 16, 2022
4bd44be
reset to known working point
snowystinger Sep 28, 2022
41de2e7
remove aria from stately
snowystinger Sep 28, 2022
3672fb2
Use relayoutNow when possible
snowystinger Sep 28, 2022
d0e10d5
Merge branch 'main' into fix-table-renders
snowystinger Sep 28, 2022
0f0b5d6
Merge branch 'main' into fix-table-renders
snowystinger Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 49 additions & 18 deletions packages/@react-spectrum/table/src/TableView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ import {GridNode} from '@react-types/grid';
// @ts-ignore
import intlMessages from '../intl/*.json';
import {Item, Menu, MenuTrigger} from '@react-spectrum/menu';
import {layoutInfoToStyle, ScrollView, setScrollLeft, useVirtualizer, VirtualizerItem} from '@react-aria/virtualizer';
import {
layoutInfoToStyle,
ScrollView,
setScrollLeft,
useVirtualizer,
VirtualizerItem
} from '@react-aria/virtualizer';
import {Nubbin} from './Nubbin';
import {ProgressCircle} from '@react-spectrum/progress';
import React, {ReactElement, useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react';
Expand Down Expand Up @@ -121,10 +127,6 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H
selectionBehavior: props.selectionStyle === 'highlight' ? 'replace' : 'toggle'
});

const columnState = useTableColumnResizeState({...mergeProps(props, {onColumnResizeEnd: () => {
setIsInResizeMode(false);
}}), getDefaultWidth}, state.collection);

// If the selection behavior changes in state, we need to update showSelectionCheckboxes here due to the circular dependency...
let shouldShowCheckboxes = state.selectionManager.selectionBehavior !== 'replace';
if (shouldShowCheckboxes !== showSelectionCheckboxes) {
Expand All @@ -134,6 +136,7 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H
let domRef = useDOMRef(ref);
let headerRef = useRef<HTMLDivElement>();
let bodyRef = useRef<HTMLDivElement>();
let virtualizer = useRef(null);
let stringFormatter = useLocalizedStringFormatter(intlMessages);

let density = props.density || 'regular';
Expand All @@ -154,6 +157,19 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H
}),
[props.overflowMode, scale, density]
);

const columnState = useTableColumnResizeState({...mergeProps(props, {
onResize: (inRender) => {
if (virtualizer.current && inRender) {
virtualizer.current.relayout({sizeChanged: true});
} else if (virtualizer.current) {
virtualizer.current.relayoutNow({sizeChanged: true});
}
},
onColumnResizeEnd: () => {
setIsInResizeMode(false);
}
}), getDefaultWidth}, state.collection);
layout.collection = state.collection;
layout.getColumnWidth = columnState.getColumnWidth;

Expand All @@ -165,7 +181,7 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H
}, state, domRef);
let [headerRowHovered, setHeaderRowHovered] = useState(false);

// This overrides collection view's renderWrapper to support DOM heirarchy.
// This overrides collection view's renderWrapper to support DOM hierarchy.
type View = ReusableView<GridNode<T>, unknown>;
let renderWrapper = (parent: View, reusableView: View, children: View[], renderChildren: (views: View[]) => ReactElement[]) => {
let style = layoutInfoToStyle(reusableView.layoutInfo, direction, parent && parent.layoutInfo);
Expand Down Expand Up @@ -366,14 +382,29 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H
headerRef={headerRef}
lastResizeInteractionModality={lastResizeInteractionModality}
bodyRef={bodyRef}
isFocusVisible={isFocusVisible}
getColumnWidth={columnState.getColumnWidth} />
virtualizer={virtualizer}
isFocusVisible={isFocusVisible} />
</TableContext.Provider>
);
}

// This is a custom Virtualizer that also has a header that syncs its scroll position with the body.
function TableVirtualizer({layout, collection, lastResizeInteractionModality, focusedKey, renderView, renderWrapper, domRef, bodyRef, headerRef, setTableWidth, getColumnWidth, onVisibleRectChange: onVisibleRectChangeProp, isFocusVisible, ...otherProps}) {
function TableVirtualizer({
layout,
collection,
lastResizeInteractionModality,
focusedKey,
renderView,
renderWrapper,
domRef,
bodyRef,
headerRef,
setTableWidth,
onVisibleRectChange: onVisibleRectChangeProp,
isFocusVisible,
virtualizer,
...otherProps
}) {
let {direction} = useLocale();
let {state: tableState, columnState} = useTableContext();
let loadingState = collection.body.props.loadingState;
Expand All @@ -390,6 +421,7 @@ function TableVirtualizer({layout, collection, lastResizeInteractionModality, fo
},
transitionDuration: isLoading ? 160 : 220
});
virtualizer.current = state.virtualizer;

let {virtualizerProps} = useVirtualizer({
focusedKey,
Expand All @@ -409,11 +441,6 @@ function TableVirtualizer({layout, collection, lastResizeInteractionModality, fo
}
}, state, domRef);

// If columnwidths change, need to relayout.
useLayoutEffect(() => {
state.virtualizer.relayoutNow({sizeChanged: true});
}, [getColumnWidth, state.virtualizer]);

useEffect(() => {
if (lastResizeInteractionModality.current === 'keyboard' && headerRef.current.contains(document.activeElement)) {
document.activeElement?.scrollIntoView?.(false);
Expand All @@ -430,9 +457,11 @@ function TableVirtualizer({layout, collection, lastResizeInteractionModality, fo
}, [bodyRef, headerRef]);

let onVisibleRectChange = useCallback((rect: Rect) => {
setTableWidth(rect.width);

// order matters here, setTableWidth calls a relayoutNow
// setVisibleRect calls a relayout after a raf, to combine them, they must be in this order
// so that the relayoutNow cancels the raf
state.setVisibleRect(rect);
setTableWidth(rect.width);

if (!isLoading && onLoadMore) {
let scrollOffset = state.virtualizer.contentSize.height - rect.height * 2;
Expand All @@ -442,13 +471,15 @@ function TableVirtualizer({layout, collection, lastResizeInteractionModality, fo
}
}, [onLoadMore, isLoading, state.setVisibleRect, state.virtualizer]);

let prevContentSizeHeight = useRef(state.virtualizer.contentSize.height);
useLayoutEffect(() => {
if (!isLoading && onLoadMore && !state.isAnimating) {
if (state.contentSize.height <= state.virtualizer.visibleRect.height) {
if (prevContentSizeHeight.current > 0 && prevContentSizeHeight.current < state.virtualizer.visibleRect.height) {
onLoadMore();
}
}
}, [state.contentSize, state.virtualizer, state.isAnimating, onLoadMore, isLoading]);
prevContentSizeHeight.current = state.virtualizer.contentSize.height;
});

let keysBefore = [];
let key = columnState.currentlyResizingColumn;
Expand Down
114 changes: 64 additions & 50 deletions packages/@react-spectrum/table/test/Table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,17 +144,26 @@ describe('TableView', function () {
act(() => {jest.runAllTimers();});
});

let render = (children, scale = 'medium') => renderComponent(
<Provider theme={theme} scale={scale}>
{children}
</Provider>
);
let render = (children, scale = 'medium') => {
let tree = renderComponent(
<Provider theme={theme} scale={scale}>
{children}
</Provider>
);
// account for table column resizing to do initial pass due to relayout from useTableColumnResizeState render
act(() => {jest.runAllTimers();});
return tree;
};

let rerender = (tree, children, scale = 'medium') => tree.rerender(
<Provider theme={theme} scale={scale}>
{children}
</Provider>
);
let rerender = (tree, children, scale = 'medium') => {
let newTree = tree.rerender(
<Provider theme={theme} scale={scale}>
{children}
</Provider>
);
act(() => {jest.runAllTimers();});
return newTree;
};

// I'd use tree.getByRole(role, {name: text}) here, but it's unbearably slow.
let getCell = (tree, text) => {
Expand Down Expand Up @@ -786,42 +795,50 @@ describe('TableView', function () {

describe('keyboard focus', function () {
// locale is being set here, since we can't nest them, use original render function
let renderTable = (locale = 'en-US', props = {}) => renderComponent(
<Provider locale={locale} theme={theme}>
<TableView aria-label="Table" {...props}>
<TableHeader columns={columns}>
{column => <Column>{column.name}</Column>}
</TableHeader>
<TableBody items={items}>
{item =>
(<Row key={item.foo}>
{key => <Cell>{item[key]}</Cell>}
</Row>)
}
</TableBody>
</TableView>
</Provider>
);
let renderTable = (locale = 'en-US', props = {}) => {
let tree = renderComponent(
<Provider locale={locale} theme={theme}>
<TableView aria-label="Table" {...props}>
<TableHeader columns={columns}>
{column => <Column>{column.name}</Column>}
</TableHeader>
<TableBody items={items}>
{item =>
(<Row key={item.foo}>
{key => <Cell>{item[key]}</Cell>}
</Row>)
}
</TableBody>
</TableView>
</Provider>
);
act(() => {jest.runAllTimers();});
return tree;
};

// locale is being set here, since we can't nest them, use original render function
let renderNested = (locale = 'en-US') => renderComponent(
<Provider locale={locale} theme={theme}>
<TableView aria-label="Table">
<TableHeader columns={nestedColumns}>
{column =>
<Column childColumns={column.children}>{column.name}</Column>
}
</TableHeader>
<TableBody items={items}>
{item =>
(<Row key={item.foo}>
{key => <Cell>{item[key]}</Cell>}
</Row>)
}
</TableBody>
</TableView>
</Provider>
);
let renderNested = (locale = 'en-US') => {
let tree = renderComponent(
<Provider locale={locale} theme={theme}>
<TableView aria-label="Table">
<TableHeader columns={nestedColumns}>
{column =>
<Column childColumns={column.children}>{column.name}</Column>
}
</TableHeader>
<TableBody items={items}>
{item =>
(<Row key={item.foo}>
{key => <Cell>{item[key]}</Cell>}
</Row>)
}
</TableBody>
</TableView>
</Provider>
);
act(() => {jest.runAllTimers();});
return tree;
};

let renderMany = () => render(
<TableView aria-label="Table">
Expand Down Expand Up @@ -3866,7 +3883,7 @@ describe('TableView', function () {
fireEvent.scroll(scrollView);
act(() => {jest.runAllTimers();});

expect(onLoadMore).toHaveBeenCalledTimes(3);
expect(onLoadMore).toHaveBeenCalledTimes(1);
Copy link
Copy Markdown
Collaborator

@ktabors ktabors Oct 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see how the first one is not scrolling far enough to cause a onLoadMore to call, but shouldn't the second one? I'd want to see expect(onLoadMore).toHaveBeenCalledTimes(0); after the first two scrolls. Should there be a test after this that does call onLoadMore twice?

});

it('should automatically fire onLoadMore if there aren\'t enough items to fill the Table', function () {
Expand All @@ -3890,11 +3907,8 @@ describe('TableView', function () {
);

render(<TableMock items={items} />);
act(() => jest.runAllTimers());
// first loadMore triggered by onVisibleRectChange, other 3 by useLayoutEffect
// we can't get better results than that without mocking every single clientHeight/Width
// this is a good candidate for storybook interactions test
expect(onLoadMoreSpy).toHaveBeenCalledTimes(4);

expect(onLoadMoreSpy).toHaveBeenCalledTimes(2);
});
});

Expand Down
Loading