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

VirtualScroller: does not update items if only rows length changes #6639

Closed
mamsellem opened this issue May 17, 2024 · 9 comments · Fixed by #6643, leoo1992/GeradorQRCode#40 or mtech-31-quemistry/quemistry_client_web#3
Assignees
Labels
Type: Bug Issue contains a defect related to a specific component.
Milestone

Comments

@mamsellem
Copy link

mamsellem commented May 17, 2024

Describe the bug

VirtualScroller with both directions does not update items if the number of columns changes but not the number of rows.
The error is in useUpdateEffect that compares prev items length with current items length.
This will return false if only the number of columns has changed.

Note that itemSize needs to be set to a constant for the issue to occur.

Reproducer

Demo on stackblitz

PrimeReact version

9.6.0+

React version

18.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

No response

Steps to reproduce the behavior

No response

Expected behavior

No response

@mamsellem mamsellem added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label May 17, 2024
@melloware melloware added Status: Needs Reproducer Issue needs a runnable reproducer and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels May 18, 2024
Copy link

Please fork the Stackblitz project and create a case demonstrating your bug report. This issue will be closed if no activities in 20 days.

@mamsellem
Copy link
Author

added demonstrator projet ( see in issue description "Reproducer")

@melloware melloware added Type: Bug Issue contains a defect related to a specific component. and removed Status: Needs Reproducer Issue needs a runnable reproducer labels May 19, 2024
@melloware melloware self-assigned this May 19, 2024
@melloware melloware added this to the 10.7.0 milestone May 19, 2024
@melloware
Copy link
Member

@mamsellem PR is welcome if you have a recommended fix?

@mamsellem
Copy link
Author

sure. I will submit a PR when possible

bwamsellem pushed a commit to bwamsellem/primereact that referenced this issue May 19, 2024
@mamsellem
Copy link
Author

Hello I tried to submit a PR from my fork of primereact that it failed with error "you must be a contributor".
This is a screenshot of the submission screen
image

@melloware
Copy link
Member

Weird I have never seen that before it should allow you to open a PR from a fork no problem?

@melloware
Copy link
Member

@mamsellem i pushed PR if you want to review.

@mamsellem
Copy link
Author

mamsellem commented May 20, 2024

I don't think using deepequals is a good idea.
We are only interesteed in updating the bounds of the virtual scroller, so only the dimensions of the items matter, not the items themselves.

This was my fix for this issue:

line 570 of VirtualScroll.js

useUpdateEffect(() => {
  if (!prevProps.items || prevProps.items.length !== (props.items || []).length || (both && prevProps.items[0].length !== (props.items ? props.items[0].length : 0))) {
      init();
    }

What do you think ?

@melloware
Copy link
Member

as mentioned props.items[0] feels hacky to check the first item of the array length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment