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.

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

Fix cell order in DOM #630

Merged
merged 2 commits into from
May 31, 2022
Merged

Fix cell order in DOM #630

merged 2 commits into from
May 31, 2022

Conversation

pradeepnschrodinger
Copy link
Collaborator

@pradeepnschrodinger pradeepnschrodinger commented Apr 4, 2022

Description and Motiviation

FDT recycles rows to avoid unmounts during scrolling. This greatly gives performance but leads to rows being rendered out of order in the DOM.
This is fine visually because FDT always translates the individual rows to the correct position.

However, this implementation detail becomes a pain when the user has code that depends on DOM ordering.
For instance, in #221, the user wanted to apply a border style to each row, but the unordered row elements messes up the CSS.
Another issue is that text selection inside the grid appears to split up here and there.

Another issue which I recently found in LD was that we often have cases like selenium tests which query the text value seen in the DOM.
If the query was done on the table, then the contents will be jumbled up because the rows are unordered in the DOM.

Fix

We simply sort the list of row React elements rendered by FDT by their row index rather than their recycle "key".

How Has This Been Tested?

Tested on this codesandbox and verified both the style issue and text selection issues are gone.
Also verified this on our examples locally, and also on LD.

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.

return <div style={style}>{this._staticRowArray}</div>;

// NOTE (pradeep): Sort the rows by row index so that they appear with the right order in the DOM (see #221)
const sortedRows = _.sortBy(this._staticRowArray, (row) =>
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._staticRowArray is not ordered by row Index, as seen here.
Here, I make sortedRows which is just the sorted version of this._staticRowArray, ordered by row index.

Copy link

@eriihine eriihine May 12, 2022

Choose a reason for hiding this comment

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

@pradeepnschrodinger had some issues with selenium tests and row indexes (our tests assumed DOM order is the display order), so I tried this fix as well.

I was able to workaround the test case issues by sorting by 'props.ariaRowIndex' instead of 'props.index'.

Would it be better to 'props.ariaRowIndex' here to ensure correct DOM order vs. the display order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eriihine , thanks for the suggestion.

props.ariaRowIndex is accurate for rows in context of the whole table (because it also consider headers, footers, etc), where as props.index is only accurate within the body rows.

I think this doesn't make a difference right now because the jumbled up rows are just the body rows, and are only within the FixedDataTableBufferedRows.js component.
So our usage here should be safe, but using ariaRowIndex definitely seems safer.

I'll make the change, thanks.

Choose a reason for hiding this comment

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

@pradeepnschrodinger great, would it possible to consider to include this fix to some future v1.2.x release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eriihine , sure.
I'll backport the fix to v1.2.x as soon as this gets merged.

Choose a reason for hiding this comment

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

this would be great, thanks!

@pradeepnschrodinger pradeepnschrodinger added the Onboarding PRs or issues which serve as a good onboarding task label Apr 4, 2022
@pradeepnschrodinger pradeepnschrodinger merged commit fda8784 into v2.0-beta May 31, 2022
pradeepnschrodinger added a commit that referenced this pull request May 31, 2022
Order cells in the DOM by row-major order.
We use aria row index for the sort criteria.
@pradeepnschrodinger
Copy link
Collaborator Author

Released with v1.2.9 and v2.0.0-beta.8.

cc: @eriihine

@pradeepnschrodinger pradeepnschrodinger deleted the fix-cell-DOM-order branch June 29, 2022 17:19
pradeepnschrodinger added a commit that referenced this pull request Aug 3, 2023
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
Labels
Onboarding PRs or issues which serve as a good onboarding task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants