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.

Sign up for GitHub

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

Jump to bottom

Expose cell visibility information to cell renderers #658

Merged
merged 2 commits into from
Nov 25, 2022
Merged

Expose cell visibility information to cell renderers #658

merged 2 commits into from
Nov 25, 2022

Conversation

pradeepnschrodinger
Copy link
Collaborator

Description

Not all cells rendered by FDT are visible to the user.
eg: Cells for columns outside the viewport will be rendered if allowCellsRecycling column prop is turned off.
Similarly, if the user has specified a positive value for bufferRowCount, then FDT prerenders a given count of rows before/after the viewport.

I'm adding a new cell prop isVisible, which can be used to figure out if the rendered cell is visible to the user.

Motivation and Context

Fixes #650, where the user wanted to run some side effects for a cell based on whether it's visible or not.

How Has This Been Tested?

Tested using our local examples.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@pradeepnschrodinger pradeepnschrodinger mentioned this pull request Nov 2, 2022
Closed
pradeepnschrodinger
pradeepnschrodinger commented Nov 2, 2022
View reviewed changes
src/FixedDataTableCell.js
@@ -289,6 +305,7 @@ class FixedDataTableCell extends React.Component {
columnKey,
height,
width,
isVisible,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This exposes isVisible to the cell renderers.

src/FixedDataTableCellGroup.js
@@ -183,6 +186,7 @@ class FixedDataTableCellGroupImpl extends React.Component {
columnGroupWidth={columnGroupWidth}
pureRendering={pureRendering}
isRTL={this.props.isRTL}
isVisible={this.props.isVisible && isHorizontallyVisible}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At this component level, props.isVisible refers to whether the cell group is vertically visible.
This combined with isHorizontallyVisible determines whether the individual cell is within the viewport.

src/FixedDataTableRow.js
// if row is not visible then no need to render it
// change in visibility is handled by the parent
// if row's visibility has changed, then update it
if (this.props.visible !== nextProps.visible) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rerender the component when visibility changes, similar to the usage here.

src/FixedDataTableCell.js
if (
nextProps.isScrolling &&
this.props.rowIndex === nextProps.rowIndex &&
this.props.isVisible === nextProps.isVisible
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ensure cell is rerendered whenever visibility changes.

@pradeepnschrodinger pradeepnschrodinger merged commit 5a22caf into v1.2.x Nov 25, 2022
@pradeepnschrodinger
Copy link
Collaborator Author

Released with v.1.2.13

@Tanner-MS
Copy link
Contributor

Is this not a breaking change? If someone was using the Cell component now there is a required isVisible prop?

Also while uptaking this change I ran into the following issue:
image
Where the propType for the FixedDataTableCellDefaultDeprecated.js should be bool instead of boolean

@pradeepnschrodinger
Copy link
Collaborator Author

Is this not a breaking change? If someone was using the Cell component now there is a required isVisible prop?

Good question. The new prop is not marked as required in the exposed/optional cell renderers (FixedDataTableCellDefault and FixedDataTableCellDefaultDeprecated), so it shouldn't be an issue.

It's only required in internal components that FDT renders directly (like FixedDataTableCell), so the prop should always be defined.

Also while uptaking this change I ran into the following issue: image Where the propType for the FixedDataTableCellDefaultDeprecated.js should be bool instead of boolean

Nice catch! I can put up a hotfix and bump up the release.

pradeepnschrodinger added a commit that referenced this pull request Dec 3, 2022
@pradeepnschrodinger
Fix PropTypes typo (boolean -> bool)
bc27879 
Typo came through #658
@pradeepnschrodinger
Copy link
Collaborator Author

@Tanner-MS , I fixed the PropTypes typo through bc27879, which is available on v1.2.15.

@pradeepnschrodinger pradeepnschrodinger mentioned this pull request Jun 29, 2023
Merged
7 tasks
This was referenced Jul 26, 2023
Closed
Merged
pradeepnschrodinger added a commit that referenced this pull request Aug 3, 2023
@pradeepnschrodinger
Fix performance regressions for v1.2 (#696)
d3ff498 
Fixes regressions that came from:
* #630 - lodash util functions are slower than their native counter parts
* #628 and #658 - `shouldComponentUpdate` checks got less optimized
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mycrobe mycrobe Awaiting requested review from mycrobe

@abhisheksingh2410 abhisheksingh2410 Awaiting requested review from abhisheksingh2410

@AmanGupta2708 AmanGupta2708 Awaiting requested review from AmanGupta2708

@karry08 karry08 Awaiting requested review from karry08

@novakps novakps Awaiting requested review from novakps

@somavara somavara Awaiting requested review from somavara

@tchoiSchrodinger tchoiSchrodinger Awaiting requested review from tchoiSchrodinger

@wcjordan wcjordan Awaiting requested review from wcjordan

Assignees

@mycrobe mycrobe

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pradeepnschrodinger @Tanner-MS @mycrobe