feat: add support for dynamic row height #170

jamesonhill · 2020-06-08T18:23:11Z

This adds a new prop estimatedRowHeight, which updates the Grid component to use VariableSizeGrid from react-window instead of FixedSizeGrid. When estimatedRowHeight is provided, rowHeight is ignored and content is dynamically sized. estimatedRowHeight is used as the default height while measuring the content.

nihgwu · 2020-06-08T19:05:04Z

Thanks for your contribution, that's a great feature, I have some questions before I take some time for a detailed review

this package is supposed to support React@^16.0.0 while you are using some new api only available in 16.3.0, that would be a breaking change(although I don't think that would be a big problem)
Have you tested with frozen columns, in that mode the there would be 2 or 3 tables actually to achieve the frozen feature, so I think you need to check if the current height is the largest when setting the row height

jamesonhill · 2020-06-08T19:12:40Z

@nihgwu Hi, yes it does currently work w/ frozen columns as well as frozen rows. LeftGrid, RightGrid, and MainGrid each call setRowHeight, https://github.com/Autodesk/react-base-table/pull/170/files#diff-1ce5af19cf8e0ddaa9ce889d8ac30d42R455, and if the key already exists and new size is bigger it updates the size. I am going to push one small update now so that it's not directly mutating the state object though.

jamesonhill · 2020-06-08T19:27:40Z

@nihgwu As far as using createContext from React @16.3, I considered including that bump here but wasn't sure if it was appropriate or not. I can add it if you think so. It would be really fantastic if it could eventually get bumped to React@16.8 so that we can use hooks for new features.

nihgwu · 2020-06-08T19:16:06Z

src/BaseTable.js

@@ -315,7 +316,10 @@ class BaseTable extends React.PureComponent {
      ? dataGetter({ columns, column, columnIndex, rowData, rowIndex })
      : getValue(rowData, dataKey);
    const cellProps = { isScrolling, cellData, columns, column, columnIndex, rowData, rowIndex, container: this };
-    const cell = renderElement(cellRenderer || <TableCell className={this._prefixClass('row-cell-text')} />, cellProps);
+    const cellClasses = cn(this._prefixClass('row-cell-text'), {
+      [this._prefixClass('row-cell-text-dynamic')]: this.props.useDynamicRowHeight,


I think the dynamic should be promoted to the row(row--dynamic) and use :not(.row--dynamic) { .cell-text{} }

perhaps we should promote even further to the top level like table--dynamic, as all the rows would have the same row--dynamic className

I was thinking maybe only applying row-cell-text when not in dynamic mode. None of those properties have any affect when content is dynamically sized, so always adding them and overriding is un-necessary work.

nihgwu · 2020-06-08T19:16:42Z

src/BaseTable.js

@@ -445,8 +449,22 @@ class BaseTable extends React.PureComponent {
    );
  }

+  setRowHeight = (index, size) => {


we don't use arrow functions in a class component

Have you considered about expansion? The indices would change on expand/collapse

Expansion seems to have fine performance for me locally. I can update this to not use arrow functions.

I think i see what you were alluding to w/ expansion. It seems its not entirely correct. I'll dig into fixing it.

I think you can use rowKey as the key instead of index, as you can have totally different data with the same index

nihgwu · 2020-06-08T19:23:43Z

src/BaseTable.js

+
+    if (rowHeightMap.hasOwnProperty(index)) {
+      if (size > rowHeightMap[index]) {
+        rowHeightMap[index] = size;


If you are using index as the key, why not use an array directly?
also you should use {...map, [index]: size} to mutate the ref
and if there is no need to re-render, you don't need to always setState at the bottom

I didn't use an array bc of how rowIndex is negative for frozen rows. It seemed simpler to pass props.rowIndex directly through instead of adding logic to figure out original index.

yes, I totally forget that 👍

src/TableRow.js

nihgwu · 2020-06-08T19:30:20Z

Yes in the next major version I will use hooks, but for this version we should try to avoid breaking changes

nihgwu · 2020-06-08T19:32:30Z

For the context, I remember there is a compatible package there?

jamesonhill · 2020-06-08T19:35:50Z

Perhaps. I haven't used any but I can check to see if one exists.

nihgwu · 2020-06-09T00:19:00Z

I haven't check if we do need the new context, but here is the polyfill for it https://github.com/jamiebuilds/create-react-context

nihgwu

Great job, thanks

nihgwu · 2020-06-09T01:09:47Z

src/BaseTable.js

@@ -1168,6 +1204,7 @@ BaseTable.propTypes = {
    ExpandIcon: PropTypes.func,
    SortIndicator: PropTypes.func,
  }),
+  useDynamicRowHeight: PropTypes.bool,


maybe we could use estimatedRowHeight for initial height and scrollToRow

if this value is presented, we use VariableSizeGrid and ignore rowHeight

src/GridTable.js

src/TableRow.js

nihgwu · 2020-06-09T01:24:55Z

src/BaseTable.js

@@ -473,14 +499,18 @@ class BaseTable extends React.PureComponent {
        rowRenderer={this.renderRow}
        onScroll={this._handleScroll}
        onRowsRendered={this._handleRowsRendered}
+        useDynamicRowHeight={useDynamicRowHeight}


I think we can use rowHeight: func|number here, then we don't need to pass so many extra props

Do you think rowHeight should become optional then? If user provides number or func, we use that otherwise it's internally calculated using the TableRow.getBoundingClientRect().height that I added?

I think that's not the same case, for the BaseTable, the rowHeight could be optional as there is a default value, so actually we can remove the isRequired in the propTypes definition without breaking anything, and if we provide estimatedRowHeight we will ignore rowHeight and use estimatedRowHeight instead

for the GridTable, we use rowHeight: number as before, but rowHeight: func in dynamic mode, like rowHeight = index => rowHeightMap[index], in this way we don't need to change the props interface at all

Ok, I think i'm following. I'll push an update and see if we're on same page.

nihgwu · 2020-06-09T02:24:08Z

For get to mention that if you have tested with the infinite loading feature, I think we can use the innerRef to get the totalRowHeight instead of calculate from the count and rowHeight https://github.com/Autodesk/react-base-table/blob/master/src/BaseTable.js#L150, in that way I think we can make that feature works

nihgwu · 2020-06-17T15:17:07Z

@jamesonhill I just pushed another commit, now you can reset cache without losing performance on resizing, it looks much better now
P.S. it's me who added shouldForceUpdate to react-window 😁

jamesonhill · 2020-06-19T21:59:43Z

I haven't been able to track down what's causing it yet, but i'm seeing a little bit of "weird-ness" the first time I hover over a row (using dynamic heights). It gets smaller while hovering, then goes back to the correct size onMouseLeave and it doesn't happen on re-hover.

nihgwu · 2020-06-20T14:57:09Z

@jamesonhill I've noticed that problem too, will inspect it

nihgwu · 2020-06-20T18:48:30Z

@jamesonhill I've tracked down the issue, but just don't know why ;(, when you hover on a row the first time, the this.props.style === prevProps.style = false for the mainTable but true for the frozen tables, then the row height in the main table won't be measured, but I just don't understand how it happens, after reset, the style should be from the cache in react-window and the compare check should be true

jamesonhill · 2020-06-21T01:00:58Z

@nihgwu yea, that is odd. I'm debugging it and it even looks like they are the same unless i'm missing something.

jamesonhill · 2020-06-21T01:02:46Z

Oh, nevermind. It looks like first render is using the estimatedRowHeight values, which makes sense.

nihgwu · 2020-06-21T11:28:19Z

@jamesonhill the object value is the same but the reference is different before/after reset cache

nihgwu · 2020-06-21T16:45:55Z

@jamesonhill I've fixed the issue, and I think it's ready to be merged, please help to review the code I changed, thanks

jamesonhill · 2020-06-22T12:45:33Z

@nihgwu it looks good to me. I've done some local testing and don't see the hover issue anymore. Scrolling w/ dynamic row heights is quite fast too! I think it's ready to merge.

nihgwu · 2020-06-22T14:01:56Z

@jamesonhill Cool, let's land it, thank you so much 🎉

jamesonhill · 2020-06-22T14:14:15Z

Awesome, thanks for your assistance & guidance. This should only require a minor version bump to the npm package?

nihgwu · 2020-06-22T14:18:11Z

sure, I'm experimenting with List instead of Grid, if it's a easy switch I want to include it, or I'll release a new version soon

nihgwu · 2020-06-22T15:51:01Z

v1.10.0 is released, I've make List work but it requires api change on onScroll, so let's do it in the next major version

nihgwu · 2020-06-23T02:44:58Z

@jamesonhill I noticed an issue regarding the out sync of measurement in productionhttps://autodesk.github.io/react-base-table/examples/dynamic-row-heights, I guess it's caused by the fact that it's much faster in production that debounce of _updateRowHeights doesn't work well as expected, so it won't wait for the row in three tables be measured and updated in a single batch, if I enlarge the debounce delay, it works fine but there is noticeable lag, maybe I have use another mechanism instead of buffer

jamesonhill · 2020-06-23T12:52:47Z

hmm, I'm actually seeing it locally but not in that production example.

nihgwu · 2020-06-23T13:24:28Z

LOL, that's interesting, I'm trying to fix it now

jamesonhill added 11 commits June 4, 2020 22:15

Initial add variableSizeGrid

22e8ce9

Fix dynamic row height prop calc

203ae84

Randomize row heights

8e4c368

Remove separate component for dynamicRows

3266f22

Only wrap text for dynamicRows

21c5f39

Cleanup TableRow

1aa84d6

Cleanup GridTable

d61dd02

Cleanup BaseTable & example

f6ca0fe

Patch dynamic row heights on frozen columsn & rows

65fdf74

Use pure component

66484f0

Cleanup logs & un-used props

6f0baeb

Avoid directly mutating rowSizeMap

f22b357

nihgwu reviewed Jun 8, 2020

View reviewed changes

Recalculate rowHeight when rowData changes

f40639e

nihgwu reviewed Jun 9, 2020

View reviewed changes

jamesonhill added 7 commits June 9, 2020 07:50

Use func expression instead of arrow func

5cc68e6

Rename index to rowKey for heightMap

bfc57af

Remove un-used columnWidth prop

25dde0b

Fix remaining arrow funcs

9b70865

Bump performance by resetting cache on rowKey

20d883b

Make rowHeight optional func or number

943eb6b

Simplify props using estimatedRowHeight

d8a912d

tweak

d579671

nihgwu added 2 commits June 22, 2020 00:08

fix a weird issue on hover

c0dcf00

skip reset

a4e4ae6

nihgwu added 2 commits June 22, 2020 01:02

Merge branch 'master' into 1.0.0-patch

fec09b6

changelog

eab8053

nihgwu merged commit 784b475 into Autodesk:master Jun 22, 2020

nihgwu deleted the 1.0.0-patch branch June 22, 2020 14:02

nihgwu mentioned this pull request Jun 22, 2020

Roadmap for V2.0 #3

Open

18 tasks

SergeyVolynkin mentioned this pull request Sep 14, 2020

Dynamic Row Width feature #235

Closed

clbrec mentioned this pull request Jan 8, 2021

Dynamic Header Height feature #268

Closed

feat: add support for dynamic row height #170

feat: add support for dynamic row height #170

Uh oh!

Conversation

jamesonhill commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nihgwu commented Jun 8, 2020

Uh oh!

jamesonhill commented Jun 8, 2020

Uh oh!

jamesonhill commented Jun 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nihgwu commented Jun 8, 2020

Uh oh!

nihgwu commented Jun 8, 2020

Uh oh!

jamesonhill commented Jun 8, 2020

Uh oh!

nihgwu commented Jun 9, 2020

Uh oh!

nihgwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nihgwu commented Jun 9, 2020

Uh oh!

nihgwu commented Jun 17, 2020

Uh oh!

jamesonhill commented Jun 19, 2020

Uh oh!

nihgwu commented Jun 20, 2020

Uh oh!

nihgwu commented Jun 20, 2020

Uh oh!

jamesonhill commented Jun 21, 2020

Uh oh!

jamesonhill commented Jun 21, 2020

Uh oh!

nihgwu commented Jun 21, 2020

Uh oh!

jamesonhill commented Jun 8, 2020 •

edited

Loading

nihgwu commented Jun 22, 2020 •

edited

Loading