Improved performance on Table with a lot of columns (> 150) #942
Conversation
source/Table/Table.js
Outdated
| a11yProps.key = "Row" + rowIndex + "-" + "Col" + columnIndex; | ||
| a11yProps.className = cn('ReactVirtualized__Table__rowColumn', className); | ||
| a11yProps.style = style; | ||
| a11yProps.title = title; |
There was a problem hiding this comment.
Can you move all these props to object literal and rename a11yProps to cellProps?
source/Table/Table.js
Outdated
| a11yProps.key = "Header-Col" + index; | ||
| a11yProps.className = classNames; | ||
| a11yProps.style = style; | ||
|
|
There was a problem hiding this comment.
Same move and rename to headerProps
There was a problem hiding this comment.
To be honest, I never intended (or imagined) Table to be used with more than a dozen columns. Anything so large as 150 would be better rendered as a Grid in my mind, because it would avoid the overhead of rendering a lot of column content that isn't visible (due to horizontal scrolling).
That being said, I don't really mind the micro-optimization in this case, so long as there is a clear inline comment explaining the motivation.
I also like the renaming suggestions @TrySound made.
source/Table/Table.js
Outdated
| @@ -523,7 +525,7 @@ export default class Table extends PureComponent { | |||
| ? SortDirection.ASC | |||
| : SortDirection.DESC; | |||
|
|
|||
| const onClick = event => { | |||
| const onClick = event => { | |||
source/Table/Table.js
Outdated
| className={cn('ReactVirtualized__Table__rowColumn', className)} | ||
| style={style} | ||
| title={title}> | ||
| > |
There was a problem hiding this comment.
Looks like the crux of this fix is to avoid generating sub-optimal React.createElement boilerplate with an added function call and for-loop. But that's not obvious at a glance, and might regress later.
I'd suggest adding a comment about why it's important not to mix inline props and spread props.
source/Table/Table.js
Outdated
|
|
||
| import cn from 'classnames'; | ||
| import Column from './Column'; | ||
| import PropTypes from 'prop-types'; | ||
| import React, {PureComponent} from 'react'; | ||
| import {findDOMNode} from 'react-dom'; | ||
| import Grid, {accessibilityOverscanIndicesGetter} from '../Grid'; | ||
| import type {CellPosition} from '../Grid'; |
source/Table/Table.js
Outdated
| a11yProps.onKeyDown = onKeyDown; | ||
| ariaLabel = column.props['aria-label'] || label || dataKey; | ||
| chTabIndex = 0; | ||
| chOnClick = onClick; |
There was a problem hiding this comment.
Can you move these values to object literal?
There was a problem hiding this comment.
Never mind. Didn't see condition
| @@ -8,6 +8,7 @@ import PropTypes from 'prop-types'; | |||
| import React, {PureComponent} from 'react'; | |||
| import {findDOMNode} from 'react-dom'; | |||
| import Grid, {accessibilityOverscanIndicesGetter} from '../Grid'; | |||
|
|
|||
| @@ -426,6 +428,7 @@ export default class Table extends PureComponent { | |||
| className, | |||
| columnData, | |||
| dataKey, | |||
| title, | |||
There was a problem hiding this comment.
nit: Alpha sort prop-lists like this 😄
| @@ -482,6 +481,7 @@ export default class Table extends PureComponent { | |||
| headerRenderer, | |||
| id, | |||
| label, | |||
| title, | |||
source/Table/Table.js
Outdated
|
|
||
| let chOnClick, cOnKeyDown, chTabIndex, ariaSort, ariaLabel; |
There was a problem hiding this comment.
What does "ch0n" and "c0n" and "ch" mean?
RV doesn't have an official styleguide, but if it did- "avoid abbreviated variable names" would be one of the first things I'd write. Please rename these.
|
|
||
| // NOTE: is important to NOT mixin inline property and spread properties, | ||
| // since internally REACT clone property object to adds inline property | ||
| // and slow down performances |
There was a problem hiding this comment.
The wording of this comment (and the one below) confuse me. I don't think this has anything to do with "React internals" (unless I'm misunderstanding something). I believe this change is being made to avoid the overhead added by the babel-runtime extends method.
Before this change:
return _react2.default.createElement(
'div',
(0, _extends3.default)({}, a11yProps, {
key: 'Row' + rowIndex + '-Col' + columnIndex,
className: (0, _classnames2.default)('ReactVirtualized__Table__rowColumn', className),
style: style,
title: title }),
renderedCell
);After this change:
return (
<div
{...columnProps}
>
{renderedCell}
</div>
);There was a problem hiding this comment.
That being said, I'm happy to update the wording of the comment myself, so don't worry about it. 😄
|
Thanks for the contribution! |
Improved performance on Table with a lot of columns, avoiding internal use of
_extends()on_createColumn().