Uxd 1984 Data Table updates on data change #1233

tristanjasper · 2022-02-22T23:48:09Z

Purpose 🚀

Currently when the source data prop of a table changes:
a) A dataTable without the Infinite loader sub component will not show a vertical scrollbar
b) A dataTable with the Infinite loader will not evaluate the correct row heights, making long text strings get cut off.

This pr is a proposal to have the dataTable update when data changes and

Re-evaluate the row heights based on row data changes, leading to long strings being cut off.
Resize the outer container div meaning additional rows will not be hidden

This pr ensures that DataTable resizes and re-renders correctly when the source data is changed.

Note
Additionally discovered a bug which i'll create an issue for-
On the Data change story
Click "Add Item" button three times
Then click "Save"
The DataTable with InfiniteLoader renders with a vertical scrollbar even though it is below the maxHeight value.

Notes ✏️

details of code change / secondary purposes of this PR

Updates 📦

If you have changed a component's source code (not stories, specs, or docs), before merging your branch run yarn changeset. This will prompt you to:

indicate if changes were patch/minor/major for each modified package
enter a release message

Storybook 📕

http://storybooks.highbond-s3.com/paprika/your-branch-name

Screenshots 📸

optional but highly recommended

References 🔗

relevant Jira ticket / GitHub issues

changeset-bot · 2022-02-22T23:48:11Z

🦋 Changeset detected

Latest commit: 875d358

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@paprika/data-table	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

tristanjasper · 2022-02-22T23:54:20Z

packages/DataTable/src/DataTable.styles.ts

    width: ${`${width}px`};
    ${fontSize()}

+    overflow: ${hasInfiniteLoader ? `hidden;` : `auto;`}


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

tristanjasper · 2022-02-23T00:25:49Z

packages/DataTable/src/components/InfiniteLoader/useItemSizeCalculator.tsx

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


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

tristanjasper · 2022-02-23T19:37:15Z

packages/DataTable/src/components/InfiniteLoader/InfiniteLoader.tsx

+      }
+    }
+    prevData.current = data;
+    isLoadingMoreItemsRef.current = false;


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

tristanjasper · 2022-02-23T19:37:46Z

packages/DataTable/src/components/InfiniteLoader/InfiniteLoader.tsx

+      if (changedIndexes.length > 0) {
+        clearRowHeights(changedIndexes);
+        listRef.current.resetAfterIndex(changedIndexes[0]);
+        setTimeout(resetDimension, 0);


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

tristanjasper · 2022-02-23T20:23:05Z

packages/DataTable/src/components/InfiniteLoader/InfiniteLoader.tsx

+        listRef.current.resetAfterIndex(changedIndexes[0]);
+        setTimeout(resetDimension, 0);
+      } else if (data.length !== prevData.current.length) {
+        resetDimension();


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)

allison-c · 2022-02-23T20:45:43Z

packages/DataTable/src/hooks/useBestTableDimensions.ts

@@ -72,5 +72,5 @@ export default function useBestTableDimensions({
    resize: resetDimension,
  }));

-  return dimensions;
+  return { dimensions, resetDimension };


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

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?

allison-c · 2022-02-23T20:46:51Z

packages/DataTable/stories/examples/DataChange.tsx

+          isItemLoaded={index => items[index] !== undefined}
+          loadMoreItems={async () => {
+            console.log("callign loadmore items");
+            // do nothing


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

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.

allison-c · 2022-02-23T21:10:28Z

packages/DataTable/src/components/InfiniteLoader/InfiniteLoader.tsx

+        if (!isEqual(row, data[index])) {
+          changedIndexes.push(index);
+        }
+      });


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!

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.

tristanjasper · 2022-02-23T22:15:07Z

Added an issue for the height bug as mentioned in the description here

tristanjasper added 3 commits February 18, 2022 15:46

refactor(DataTable): UXD-1984 evaluate row height on data change

b383d5d

fix(DataTable): UXD-1984 dataTable adapts to data changes correctly

fe91a9a

uxd-1984-small-refactor

e2c4eeb

tristanjasper changed the title ~~Uxd 1984 Data Table updates on data change~~ Uxd 1984 Data Table updates on data change - WIP Feb 22, 2022

tristanjasper added the WIP label Feb 22, 2022

tristanjasper added 4 commits February 22, 2022 16:21

refactor(DataTable): UXD-1984 small change

21af5cb

refactor(DataTable): update data change logic

306a9d8

Merge branch 'master' into uxd-1984-data-table-change-on-data

4b1529b

refactor(DataTable): remove vertical scrollbar logic

c317a2a

tristanjasper commented Feb 23, 2022

View reviewed changes

tristanjasper removed the WIP label Feb 23, 2022

tristanjasper requested review from allison-c and AndreyChernykh February 23, 2022 19:57

tristanjasper changed the title ~~Uxd 1984 Data Table updates on data change - WIP~~ Uxd 1984 Data Table updates on data change Feb 23, 2022

tristanjasper marked this pull request as ready for review February 23, 2022 20:13

chore(DataTable): UXD-1984 add changeset

f2f1efd

tristanjasper commented Feb 23, 2022

View reviewed changes

allison-c reviewed Feb 23, 2022

View reviewed changes

fix(DataTable): UXD-1984 small resize fix

875d358

AndreyChernykh approved these changes Feb 23, 2022

View reviewed changes

tristanjasper merged commit d3c2e23 into master Feb 23, 2022

tristanjasper deleted the uxd-1984-data-table-change-on-data branch February 23, 2022 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uxd 1984 Data Table updates on data change #1233

Uxd 1984 Data Table updates on data change #1233

tristanjasper commented Feb 22, 2022 •

edited

Loading

changeset-bot bot commented Feb 22, 2022 •

edited

Loading

tristanjasper Feb 22, 2022

tristanjasper Feb 23, 2022

tristanjasper Feb 23, 2022

tristanjasper Feb 23, 2022

tristanjasper Feb 23, 2022

allison-c Feb 23, 2022

tristanjasper Feb 23, 2022

allison-c Feb 23, 2022

tristanjasper Feb 23, 2022

allison-c Feb 23, 2022 •

edited

Loading

tristanjasper Feb 23, 2022

tristanjasper commented Feb 23, 2022

Uxd 1984 Data Table updates on data change #1233

Uxd 1984 Data Table updates on data change #1233

Conversation

tristanjasper commented Feb 22, 2022 • edited Loading

Purpose 🚀

Notes ✏️

Updates 📦

Storybook 📕

Screenshots 📸

References 🔗

changeset-bot bot commented Feb 22, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allison-c Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristanjasper commented Feb 23, 2022

tristanjasper commented Feb 22, 2022 •

edited

Loading

changeset-bot bot commented Feb 22, 2022 •

edited

Loading

allison-c Feb 23, 2022 •

edited

Loading