Repository navigation
Conversation
Hey! Thanks! I love getting quality PRs. 😁
This should not be the case if you |
bvaughn
left a comment
bvaughn
left a comment
There was a problem hiding this comment.
I don't know how I feel about this change.
On the one hand, I love the idea of supporting horizontal scrolling in Table. It has come up before a time or two. I've always backed away from it though for fear of breaking existing use cases. Table has always been resize-to-fit at its core. For example, the Column width prop docs say:
Flex basis (width) for this column; This value can grow or shrink based on :flexGrow and :flexShrink properties.
I think something like this proposed change will require an explicit opt-in (eg a enableHorizontalScroll boolean prop).
| className={cn('ReactVirtualized__Table__Grid', gridClassName)} | ||
| cellRenderer={this._createRow} | ||
| columnWidth={width} | ||
| columnWidth={totalColumnWidth} |
There was a problem hiding this comment.
This should prob be Math.max(width, totalColumnWidth) no? Since Table columns are supposed to grow to fit. (Would you ever want your row filling up only part of the width of your table?)
There was a problem hiding this comment.
Otherwise tables may end up looking like the demo site does now (with this branch):
There was a problem hiding this comment.
Yes, you are right. We'll change that on our side.
|
I'm seeing some funky behavior if I update the table example to test this. Let's say I set 23 columns total. I'm only able to scroll to see the 21st column. Here's a quick video demo: It's possible my local edits have caused this, though...I wouldn't initially expect that. Edit It's the |
|
I find myself feeling like |
|
Here's a patch for you to apply and play with, if you're interested in continuing the investigation with diff --git a/source/Table/Table.example.js b/source/Table/Table.example.js
index 1eddad9..f6333eb 100644
--- a/source/Table/Table.example.js
+++ b/source/Table/Table.example.js
@@ -20,6 +20,7 @@ export default class TableExample extends PureComponent {
this.state = {
disableHeader: false,
+ enableHorizontalScroll: false,
headerHeight: 30,
height: 270,
hideIndexRow: false,
@@ -44,6 +45,7 @@ export default class TableExample extends PureComponent {
render () {
const {
disableHeader,
+ enableHorizontalScroll,
headerHeight,
height,
hideIndexRow,
@@ -117,6 +119,17 @@ export default class TableExample extends PureComponent {
/>
Hide header?
</label>
+
+ <label className={styles.checkboxLabel}>
+ <input
+ aria-label='Enable horizontal scroll?'
+ checked={enableHorizontalScroll}
+ className={styles.checkbox}
+ type='checkbox'
+ onChange={event => this.setState({ enableHorizontalScroll: event.target.checked })}
+ />
+ Enable horizontal scroll?
+ </label>
</ContentBoxParagraph>
<InputRow>
@@ -166,6 +179,7 @@ export default class TableExample extends PureComponent {
<Table
ref='Table'
disableHeader={disableHeader}
+ enableHorizontalScroll={enableHorizontalScroll}
headerClassName={styles.headerColumn}
headerHeight={headerHeight}
height={height}
@@ -189,17 +203,18 @@ export default class TableExample extends PureComponent {
}
dataKey='index'
disableSort={!this._isSortEnabled()}
+ flexShrink={enableHorizontalScroll ? 0 : 1}
width={60}
/>
}
<Column
dataKey='name'
disableSort={!this._isSortEnabled()}
+ flexShrink={enableHorizontalScroll ? 0 : 1}
headerRenderer={this._headerRenderer}
width={90}
/>
<Column
- width={210}
disableSort
label='The description label is really long so that it will be truncated'
dataKey='random'
@@ -208,7 +223,10 @@ export default class TableExample extends PureComponent {
({ cellData, columnData, dataKey, rowData, rowIndex }) => cellData
}
flexGrow={1}
+ flexShrink={enableHorizontalScroll ? 0 : 1}
+ width={210}
/>
+ {this._getMoreColumns()}
</Table>
)}
</AutoSizer>
@@ -221,6 +239,34 @@ export default class TableExample extends PureComponent {
return list.get(index % list.size)
}
+ _getMoreColumns () {
+ const { enableHorizontalScroll } = this.state
+
+ const columns = []
+
+ if (enableHorizontalScroll) {
+ for (let i = 0; i < 15; i++) {
+ columns.push(
+ <Column
+ cellDataGetter={
+ ({ columnData, dataKey, rowData }) => i
+ }
+ cellRenderer={
+ ({ cellData, columnData, dataKey, rowData, rowIndex }) => `Column ${i}`
+ }
+ dataKey={`${i}`}
+ key={i}
+ label={`Column ${i}`}
+ flexShrink={0}
+ width={100}
+ />
+ )
+ }
+ }
+
+ return columns
+ }
+
_getRowHeight ({ index }) {
const { list } = this.context
diff --git a/source/Table/Table.js b/source/Table/Table.js
index 6c8c81e..a389351 100644
--- a/source/Table/Table.js
+++ b/source/Table/Table.js
@@ -38,6 +38,9 @@ export default class Table extends PureComponent {
/** Disable rendering the header at all */
disableHeader: PropTypes.bool,
+ /** Enable horizontal scrolling */
+ enableHorizontalScroll: PropTypes.bool.isRequired,
+
/**
* Used to estimate the total height of a Table before all of its rows have actually been measured.
* The estimated total height is adjusted as rows are rendered.
@@ -202,6 +205,7 @@ export default class Table extends PureComponent {
static defaultProps = {
disableHeader: false,
+ enableHorizontalScroll: false,
estimatedRowSize: 30,
headerHeight: 0,
headerStyle: {},
@@ -292,9 +296,8 @@ export default class Table extends PureComponent {
// Precompute and cache column styles before rendering rows and columns to speed things up
this._cachedColumnStyles = []
- let totalColumnWidth = 0
+
React.Children.toArray(children).forEach((column, index) => {
- totalColumnWidth += column.props.width
const flexStyles = this._getFlexStyleForColumn(column, column.props.style)
this._cachedColumnStyles[index] = {
@@ -303,6 +306,8 @@ export default class Table extends PureComponent {
}
})
+ const columnWidth = this._getColumnWidth()
+
// Note that we specify :rowCount, :scrollbarWidth, :sortBy, and :sortDirection as properties on Grid even though these have nothing to do with Grid.
// This is done because Grid is a pure component and won't update unless its properties or state has changed.
// Any property that should trigger a re-render of Grid then is specified here to avoid a stale display.
@@ -310,7 +315,12 @@ export default class Table extends PureComponent {
<div
className={cn('ReactVirtualized__Table', className)}
id={id}
- style={style}
+ style={{
+ ...style,
+ height,
+ overflow: 'hidden',
+ width,
+ }}
>
{!disableHeader && (
headerRowRenderer({
@@ -322,7 +332,7 @@ export default class Table extends PureComponent {
overflow: 'hidden',
paddingRight: scrollbarWidth,
transform: `translate(${headerRowTranslateX}px, 0px)`,
- width: width
+ width: columnWidth
}
})
)}
@@ -332,7 +342,7 @@ export default class Table extends PureComponent {
autoContainerWidth
className={cn('ReactVirtualized__Table__Grid', gridClassName)}
cellRenderer={this._createRow}
- columnWidth={totalColumnWidth}
+ columnWidth={columnWidth}
columnCount={1}
height={availableRowsHeight}
id={undefined}
@@ -475,10 +485,8 @@ export default class Table extends PureComponent {
const rowStyleObject = typeof rowStyle === 'function' ? rowStyle({ index }) : rowStyle
const rowData = rowGetter({ index })
- let rowWidth = 0
const columns = React.Children.toArray(children).map(
(column, columnIndex) => {
- rowWidth += column.props.width
return this._createColumn({
column,
columnIndex,
@@ -495,7 +503,6 @@ export default class Table extends PureComponent {
const flattenedStyle = {
...style,
...rowStyleObject,
- width: rowWidth,
height: this._getRowHeight(index),
overflow: 'hidden',
paddingRight: scrollbarWidth
@@ -516,6 +523,26 @@ export default class Table extends PureComponent {
})
}
+ _getColumnWidth () {
+ const {
+ children,
+ enableHorizontalScroll,
+ width
+ } = this.props
+
+ const { scrollbarWidth } = this.state
+
+ let totalColumnWidth = 0
+
+ if (enableHorizontalScroll) {
+ React.Children.forEach(children, (column) => {
+ totalColumnWidth += column.props.width
+ })
+ }
+
+ return Math.max(width, totalColumnWidth) - scrollbarWidth
+ }
+
/**
* Determines the flex-shrink, flex-grow, and width values for a cell (header or column).
*/
|
|
I'm fairly confident that the best way forward here is to consider using the |
|
Sure. I don't have a problem with making this behavior opt-in if that helps allay concerns about breaking existing use cases. I was just surprised after working with Table that it did not have support for this use case, which is important to us since we have many use cases where we have tables that have just a few more columns than can fit on a reasonably sized browser page at once, and for which we wanted a horizontal scroll. If using MultiGrid instead of Table is the recommended way to get support for horizontally scrolling table-style UI, then we will switch to using it in our implementation so we don't need to worry about the stability over time of the hacks we employed to translate the header. But I personally think that if the door is closed to support this in Table, then there should perhaps be a more prominent warning in the documentation about the lack of support for horizontal scrolling in Table and that if you will need horizontal scrolling now or in the future, you should use MultiGrid instead. I think there may be a small chance that if you consider that, you might decide it makes sense to keep this behavior in Table as an opt-in. Thanks! |
|
Happy to add a warning to the I would also be happy to reconsider merging horizontal scroll support into Anyway, in my mind, the purpose of I hope you're able to integrate with that component easily. I'm happy to help (in Slack) if you run into any questions though! |
|
Sorry for the very long delay in getting back to you about this! I've just finished a large update for version 10 of react-virtualized (#1900): the library has been rewritten in TypeScript, every component is now a function component (React 18+ is required), and the source moved from If you're still interested in this change, I'd welcome a new PR with it rebased onto the |
2 participants
Hey! We love your react-virtualized tables. We recently migrated from fixed-data-table to react-virtualized, but we discovered that react-virtualized tables don't support horizontal scrolling. We created some hacky fixes in our project to deal with this (forcing overflowX to on and cleaning up the row widths and header translation in a componentDidUpdate), but I wanted to contribute these enhancements back to react-virtualized itself!
Note that I ran npm test and got some failures which were pre-existing before my changes. I did make a minor change to fix something which did actually break as a result of these changes.