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
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
6 changes: 4 additions & 2 deletions packages/DataTable/src/DataTable.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,18 @@ 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;
width: ${`${width}px`};
${fontSize()}

overflow: ${hasInfiniteLoader ? `hidden;` : `auto;`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensures the vertical scroll bar is showing when data changes and rendering without the hasInfiniteLoader sub component


* {
box-sizing: inherit;
}
Expand Down
9 changes: 5 additions & 4 deletions packages/DataTable/src/DataTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ interface DataTableComposition {
InfiniteLoader: React.FC<InfiniteLoaderPublicProps>;
types: ConstantsTypes;
}

export interface DataTableProps {
/** Accessible description of the table */
a11yText: string;
Expand Down Expand Up @@ -95,7 +94,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 +126,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,7 +138,9 @@ function DataTable(
height={bestDimensions.height}
innerElementType={InnerElement}
getRowHeight={getRowHeight}
resetDimension={resetDimension}
shouldHaveHorizontalScroll={bestDimensions.shouldHaveHorizontalScroll}
shouldHaveVerticalScroll={bestDimensions.shouldHaveVerticalScroll}
{...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,7 @@
import React from "react";
import { ListChildComponentProps, VariableSizeList } from "react-window";
import isEqual from "lodash.isequal";
import { usePrevious } from "@paprika/helpers";
import ReactInfiniteLoader from "react-window-infinite-loader";
import TableRow from "../TableRow/TableRow";
import { TableDataItemType } from "../../types";
Expand Down Expand Up @@ -39,6 +41,8 @@ interface InfiniteLoaderPrivateProps {
height: number;
getRowHeight: ((index: number) => number) | null;
shouldHaveHorizontalScroll: boolean;
shouldHaveVerticalScroll: boolean;
resetDimension: () => void;
}

function Row(props: ListChildComponentProps): JSX.Element {
Expand All @@ -60,15 +64,44 @@ export function InfiniteLoaderImpl({
itemCount,
loadMoreItems,
shouldHaveHorizontalScroll,
shouldHaveVerticalScroll,
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 { getItemSize, clearRowHeights } = useItemSizeCalculator(data, getRowHeight);

const prevData: TableDataItemType[] | undefined = usePrevious(data);

React.useLayoutEffect(() => {
if (!isLoadingMoreItemsRef.current && prevData && listRef.current) {
if (!shouldHaveVerticalScroll && data.length !== prevData.length) {
isLoadingMoreItemsRef.current = true;
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)

} else {
const changedIndexes: number[] = [];
prevData.forEach((row: TableDataItemType, index: number) => {
if (!isEqual(row, data[index])) {
changedIndexes.push(index);
}
});
if (changedIndexes.length > 0) {
clearRowHeights(changedIndexes);
listRef.current.resetAfterIndex(changedIndexes[0]);
setTimeout(resetDimension, 100);
}
}
} else {
isLoadingMoreItemsRef.current = false;
}
}, [data, prevData, clearRowHeights, resetDimension, shouldHaveVerticalScroll]);

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

if (listRef.current) {
Expand All @@ -94,7 +127,10 @@ export function InfiniteLoaderImpl({
onItemsRendered={onItemsRendered}
ref={listRef}
innerElementType={innerElementType}
style={{ overflowX: shouldHaveHorizontalScroll ? "auto" : "hidden" }}
style={{
overflowX: shouldHaveHorizontalScroll ? "auto" : "hidden",
overflowY: shouldHaveVerticalScroll ? "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 };
}
20 changes: 12 additions & 8 deletions packages/DataTable/src/hooks/useBestTableDimensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ type Dimensions = {
width: number;
height: number;
shouldHaveHorizontalScroll: boolean;
shouldHaveVerticalScroll: boolean;
};

export default function useBestTableDimensions({
Expand All @@ -22,13 +23,17 @@ 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),
shouldHaveHorizontalScroll: false,
shouldHaveVerticalScroll: false,
}));

const maxWidthInNumber = React.useCallback(() => convertSizeStringToNumber(maxWidth, Direction.width), [maxWidth]);
const maxHeightInNumber = React.useCallback(() => convertSizeStringToNumber(maxHeight, Direction.width), [maxHeight]);

const resetDimension = React.useCallback(() => {
if (!tableRef.current) return;

Expand All @@ -42,16 +47,15 @@ export default function useBestTableDimensions({
setDimensions(() => {
const realWidth = theadEl.clientWidth;
const realHeight = theadEl.clientHeight + tbodyEl.clientHeight;
const maxWidthInNumber = convertSizeStringToNumber(maxWidth, Direction.width);
const maxHeightInNumber = convertSizeStringToNumber(maxHeight, Direction.height);

return {
width: Math.min(maxWidthInNumber, realWidth),
height: Math.min(maxHeightInNumber, realHeight),
shouldHaveHorizontalScroll: maxWidthInNumber < realWidth,
width: Math.min(maxWidthInNumber(), realWidth),
height: Math.min(maxHeightInNumber(), realHeight),
shouldHaveHorizontalScroll: maxWidthInNumber() < realWidth,
shouldHaveVerticalScroll: maxHeightInNumber() < realHeight,
};
});
}, [maxHeight, maxWidth, tableRef]);
}, [maxHeightInNumber, maxWidthInNumber, tableRef]);

React.useLayoutEffect(() => {
resetDimension();
Expand All @@ -72,5 +76,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;
11 changes: 11 additions & 0 deletions packages/helpers/src/hooks/usePrevious.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from "react";

function usePrevious<T>(value: T): T | undefined {
const ref = React.useRef<T>();
React.useEffect(() => {
ref.current = value;
}, [value]);
return ref.current;
}

export default usePrevious;
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