Skip to content
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.

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

Uxd 1984 Data Table updates on data change #1233

Merged
merged 9 commits into from
Feb 23, 2022
5 changes: 5 additions & 0 deletions .changeset/young-worms-taste.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@paprika/data-table": minor
---

DataTable updates on data change
4 changes: 4 additions & 0 deletions packages/DataTable/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,14 @@
"lodash.debounce": "^4.0.8",
"react-table": "^7.7.0",
"react-window": "^1.8.6",
"lodash.isequal": "^4.5.0",
"react-window-infinite-loader": "^1.0.7"
},
"devDependencies": {
"@paprika/input": "^4.0.17",
"@paprika/button": "^1.1.11",
"@types/lodash.debounce": "^4.0.6",
"@types/lodash.isequal": "^4.5.5",
"@types/react-table": "^7.7.7",
"@types/react-window": "^1.8.5",
"@types/react-window-infinite-loader": "^1.0.5"
Expand Down
5 changes: 3 additions & 2 deletions packages/DataTable/src/DataTable.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ import { fontSize } from "@paprika/stylers/lib/helpers";
export const Table = styled.div<{
width: number;
height: number;
hasInfiniteLoader: boolean;
}>(
({ width, height }) => css`
({ width, height, hasInfiniteLoader }) => css`
border: 1px solid ${tokens.border.color};
border-collapse: collapse;
box-sizing: border-box;
height: ${`${height}px`};
overflow: hidden;
overflow: ${hasInfiniteLoader ? `hidden;` : `auto;`}
width: ${`${width}px`};
${fontSize()}

Expand Down
7 changes: 4 additions & 3 deletions packages/DataTable/src/DataTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ function DataTable(
componentRef: React.RefObject<unknown>
) {
const tableRef = React.useRef<HTMLDivElement>(null);
const bestDimensions = useBestTableDimensions({
const { dimensions: bestDimensions, resetDimension } = useBestTableDimensions({
tableRef,
componentRef,
maxHeight,
Expand Down Expand Up @@ -127,10 +127,9 @@ function DataTable(
"DataTable.InfiniteLoader": extractedInfiniteLoaderDefinition,
} = extractChildren(children, ["DataTable.InfiniteLoader"]);
/* eslint-enable @typescript-eslint/ban-ts-comment */
const hasInfiniteLoader = Boolean(extractedInfiniteLoaderDefinition);

function renderTableContent() {
const hasInfiniteLoader = Boolean(extractedInfiniteLoaderDefinition);

if (hasInfiniteLoader) {
const infiniteLoaderPublicProps = extractedInfiniteLoaderDefinition.props as InfiniteLoaderPublicProps;

Expand All @@ -140,6 +139,7 @@ function DataTable(
height={bestDimensions.height}
innerElementType={InnerElement}
getRowHeight={getRowHeight}
resetDimension={resetDimension}
shouldHaveHorizontalScroll={bestDimensions.shouldHaveHorizontalScroll}
{...infiniteLoaderPublicProps}
/>
Expand All @@ -164,6 +164,7 @@ function DataTable(
data-pka-anchor="dataTable"
width={bestDimensions.width}
height={bestDimensions.height}
hasInfiniteLoader={hasInfiniteLoader}
{...tableInstance.getTableProps()}
{...moreProps}
>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from "react";
import { ListChildComponentProps, VariableSizeList } from "react-window";
import isEqual from "lodash.isequal";
import ReactInfiniteLoader from "react-window-infinite-loader";
import TableRow from "../TableRow/TableRow";
import { TableDataItemType } from "../../types";
Expand Down Expand Up @@ -39,6 +40,7 @@ interface InfiniteLoaderPrivateProps {
height: number;
getRowHeight: ((index: number) => number) | null;
shouldHaveHorizontalScroll: boolean;
resetDimension: () => void;
}

function Row(props: ListChildComponentProps): JSX.Element {
Expand All @@ -63,12 +65,36 @@ export function InfiniteLoaderImpl({
isNextPageLoading = false,
minimumBatchSize = 10,
threshold = 15,
resetDimension,
}: InfiniteLoaderPrivateProps & InfiniteLoaderPublicProps): JSX.Element {
const infiniteLoaderRef = React.useRef(null);
const listRef = React.useRef<VariableSizeList>(null);
const { getItemSize } = useItemSizeCalculator(data, getRowHeight);
const isLoadingMoreItemsRef = React.useRef(false);
const prevData = React.useRef<TableDataItemType[]>(data);
const { getItemSize, clearRowHeights } = useItemSizeCalculator(data, getRowHeight);

React.useLayoutEffect(() => {
if (!isLoadingMoreItemsRef.current && listRef.current) {
const changedIndexes: number[] = [];
prevData.current.forEach((row: TableDataItemType, index: number) => {
if (!isEqual(row, data[index])) {
changedIndexes.push(index);
}
});
Copy link
Contributor

@allison-c allison-c Feb 23, 2022

Choose a reason for hiding this comment

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

I just feel these calculations are quite expensive, e.g. the data from the data[index] might not always be useful. Some columns might be hidden, but the data for those columns are still in the data object passed into DataTable.

Another example is, let's say only the data of row number 2 is changed, but since we have 200 rows, we still need to check if the data for row number 3 - row number 200 have been changed

So I'd still prefer using useImperativeHandle to let the consumer decide which indexes they updated, or from which index the data are newly updated. Open for discussions!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It is expensive but my thoughts are if we don't do it here we will still need to evaluate which indexes have changed in the consumer. I would have thought letting the dataTable deal with that would be easier for developers using it.

if (changedIndexes.length > 0) {
clearRowHeights(changedIndexes);
listRef.current.resetAfterIndex(changedIndexes[0]);
setTimeout(resetDimension, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finding i've had to use setTimeout here to ensure the resize gets applied after the react table has finished resetting, I don't see a way to avoid it

} else if (data.length !== prevData.current.length) {
resetDimension();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I only call this if the height of the table is less that then maxHeight (as that is when it should only need to be called), however I discovered a bug (detailed in description) where the height of the react-table isn't always evaluated the same how is is rendered (Issue will be added)

}
}
prevData.current = data;
isLoadingMoreItemsRef.current = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensuring there are not unnecessary calculations being done when additional rows are being added by using this flag when necessary

}, [data, clearRowHeights, resetDimension]);

async function handleLoadMoreItems() {
isLoadingMoreItemsRef.current = true;
await loadMoreItems();

if (listRef.current) {
Expand All @@ -94,7 +120,9 @@ export function InfiniteLoaderImpl({
onItemsRendered={onItemsRendered}
ref={listRef}
innerElementType={innerElementType}
style={{ overflowX: shouldHaveHorizontalScroll ? "auto" : "hidden" }}
style={{
overflowX: shouldHaveHorizontalScroll ? "auto" : "hidden",
}}
>
{Row}
</VariableSizeList>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const rowHeightHelper = new RowHeightHelper();
export default function useItemSizeCalculator(
data: TableDataItemType[],
getRowHeight: ((index: number) => number) | null
): { getItemSize: (index: number) => number } {
): { getItemSize: (index: number) => number; clearRowHeights: (indexes: number[]) => void } {
const rowHeights = React.useRef<Record<number, number>>({});
const { allColumns } = useReactTableContext();

Expand All @@ -24,6 +24,11 @@ export default function useItemSizeCalculator(
[allColumns]
);

const clearRowHeights = React.useCallback(
(indexes: number[]) => indexes.forEach(index => delete rowHeights.current[index]),
[]
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only re-evaluating the data row heights for rows that have changed


function getItemSize(index: number): number {
if (!rowHeights.current[index]) {
const newRowHeight =
Expand All @@ -44,5 +49,5 @@ export default function useItemSizeCalculator(
return rowHeights.current[index];
}

return { getItemSize };
return { getItemSize, clearRowHeights };
}
4 changes: 2 additions & 2 deletions packages/DataTable/src/hooks/useBestTableDimensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export default function useBestTableDimensions({
maxHeight: string;
maxWidth: string;
shouldResizeWithViewport: boolean;
}): Dimensions {
}): { dimensions: Dimensions; resetDimension: () => void } {
const [dimensions, setDimensions] = React.useState<Dimensions>(() => ({
width: convertSizeStringToNumber(maxWidth, Direction.width),
height: convertSizeStringToNumber(maxHeight, Direction.height),
Expand Down Expand Up @@ -72,5 +72,5 @@ export default function useBestTableDimensions({
resize: resetDimension,
}));

return dimensions;
return { dimensions, resetDimension };
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're adding exports here, maybe we can move shouldResizeWithViewport out, so like { dimensions, resetDimension, shouldResizeWithViewport }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the shouldResizeWithViewport prop value is passed into this hook in dataTable.tsx. I'm not sure why we would want to expose it here as it is already available in that component. Can you expand?

}
3 changes: 3 additions & 0 deletions packages/DataTable/stories/DataTable.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import RealWorld from "./examples/RealWorld";
import RenderRow from "./examples/RenderRow";
import Showcase from "./examples/Showcase";
import WithCollapsible from "./examples/WithCollapsible";
import DataChange from "./examples/DataChange";

export default {
title: getStoryName("DataTable"),
Expand All @@ -29,3 +30,5 @@ export const realWorld = RealWorld;
export const performance = Performance;

export const withCollapsible = WithCollapsible;

export const dataChange = DataChange;
89 changes: 89 additions & 0 deletions packages/DataTable/stories/examples/DataChange.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import React from "react";
import Input from "@paprika/input";
import Button from "@paprika/button";
import DataTable from "../../src";
import makeData from "../helpers/makeData";

const DataChangeStory: () => JSX.Element = () => {
const inputRef = React.useRef<HTMLInputElement>(null);
const columns = React.useMemo(
() => [
{
Header: "First Name",
accessor: "firstName",
width: 100,
},
{
Header: "Description",
width: 100,
accessor: "desc",
},
{
Header: "Last Name",
accessor: "lastName",
width: 100,
},
{
Header: "Age",
accessor: "age",
width: 50,
},
{
Header: "Visits",
accessor: "visits",
width: 60,
},
{
Header: "Status",
accessor: "status",
},
],
[]
);

const [items, setItems] = React.useState(() => makeData(5).map(item => ({ ...item, desc: "a custom description" })));

const handleSave = () => {
setItems(prevItems => {
const newItemCopy = { ...prevItems[2] };
const inputValue = inputRef && inputRef.current ? inputRef.current.value : "";
newItemCopy.desc = inputValue;
const newItems = [...prevItems];
newItems[0] = newItemCopy;
newItems[2] = newItemCopy;
return newItems;
});
};

const handleAddItem = () => {
setItems(prevItems => {
const newItemCopy = { ...prevItems[0] };
const newItems = [...prevItems];
newItems.push(newItemCopy);
return newItems;
});
};

return (
<>
<Input
ref={inputRef}
defaultValue="This will be the description that will be set in the data on the first and third row. Click save to update the table data"
/>
<Button onClick={handleSave}>Save</Button>
<Button onClick={handleAddItem}>Add item</Button>
<DataTable a11yText="A simple data table." columns={columns} data={items}>
<DataTable.InfiniteLoader
itemCount={items.length}
isItemLoaded={index => items[index] !== undefined}
loadMoreItems={async () => {
console.log("callign loadmore items");
// do nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Loading more items is kind of a data change event as well, can we enable it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this story is about the data data object changing outside of the dataComponent. The loadMoreItems call is only triggered when the user scrolls down the table and a call to get new items can be made so isn't quite the same scenario, so I'm not sure there would be value adapting this.

}}
/>
</DataTable>
</>
);
};

export default DataChangeStory;
2 changes: 1 addition & 1 deletion packages/DataTable/stories/examples/RealWorld.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export const RealWorldStory: (props: { isForTesting?: boolean }) => JSX.Element
const [borderType, setBorderType] = React.useState(DataTable.types.borderType.GRID);
const [hasZebraStripes, setHasZebraStripes] = React.useState(false);

const { filters, filteredData, getFilterProps, getFilterItemProps } = useFilter({
const { filters, getFilterProps, getFilterItemProps } = useFilter({
columns: columnsSettings,
data,
initialState: {
Expand Down
7 changes: 7 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5508,6 +5508,13 @@
dependencies:
"@types/lodash" "*"

"@types/lodash.isequal@^4.5.5":
version "4.5.5"
resolved "https://registry.yarnpkg.com/@types/lodash.isequal/-/lodash.isequal-4.5.5.tgz#4fed1b1b00bef79e305de0352d797e9bb816c8ff"
integrity sha512-4IKbinG7MGP131wRfceK6W4E/Qt3qssEFLF30LnJbjYiSfHGGRU/Io8YxXrZX109ir+iDETC8hw8QsDijukUVg==
dependencies:
"@types/lodash" "*"

"@types/lodash@*":
version "4.14.178"
resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.178.tgz#341f6d2247db528d4a13ddbb374bcdc80406f4f8"
Expand Down