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

[DataGridPro] Fix onRowsScrollEnd being triggered instantly when using a bottom pinned row #14602

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

arminmeh
Copy link
Contributor

Fixes #12973

Having a bottom pinned row would connect the last row observer which would report intersection.
Condition updated to target the content rows only

@arminmeh arminmeh added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! plan: Pro Impact at least one Pro user feature: Row loading Related to the data grid Row loading features feature: Row pinning Related to the data grid Row pinning feature labels Sep 12, 2024
@mui-bot
Copy link

mui-bot commented Sep 12, 2024

Deploy preview: https://deploy-preview-14602--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against c185884

@arminmeh arminmeh marked this pull request as ready for review September 13, 2024 07:09
@arminmeh arminmeh requested a review from a team September 13, 2024 07:12
@arminmeh arminmeh merged commit d64e377 into mui:master Sep 13, 2024
18 checks passed
@arminmeh arminmeh deleted the fix-infinite-loading-with-row-pinning branch September 13, 2024 17:01
@@ -160,4 +160,52 @@ describe('<DataGridPro /> - Infnite loader', () => {
// should not load more rows because the threshold is not reached
expect(getRow.callCount).to.equal(5);
});

it('should not call `onRowsScrollEnd` if there are rows pinned to the bottom and the viewport scroll is at the top', async function test() {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume it is because of sleep. Is it fine if I add the fix to your PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sure 👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

Failed on this one too

Copy link
Member

Choose a reason for hiding this comment

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

Attempted to fix it in 40e09d0 (#14613)

Copy link
Contributor Author

@arminmeh arminmeh Sep 16, 2024

Choose a reason for hiding this comment

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

I had to add sleep, because the assertion expect(handleRowsScrollEnd.callCount).to.equal(0); will work fine even with the old code. Call happens a bit later, but it seems that sometimes it takes even more than 1ms to complete it. It might be connected with the IntersectionObserver cycle.
Anyway, I have found another way to deal with this and will push to your PR will create another PR to address this

Copy link
Member

Choose a reason for hiding this comment

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

I dropped the sleep because if it's called twice the test will fail anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Row loading Related to the data grid Row loading features feature: Row pinning Related to the data grid Row pinning feature plan: Pro Impact at least one Pro user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] onRowsScrollEnd triggered instantly when using a bottom pinned row
5 participants