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

Enhancement proposal: isScrolling #127

Closed
guirip opened this issue Jan 29, 2019 · 4 comments
Closed

Enhancement proposal: isScrolling #127

guirip opened this issue Jan 29, 2019 · 4 comments
Labels
💬 question Further information is requested

Comments

@guirip
Copy link

guirip commented Jan 29, 2019

Hello Brian

I've got positive feedback on the long lists since I implemented usage of react-window. Thanks again for the great work.

One of the things the users don't like is the white screen when scrolling pretty fast. To reduce this I have set prop useIsScrolling and used isScrolling row renderer property, but it doesn't seem to take into account if rows have already been rendered.

On this example if you scroll very slowly you realize that the "Scrolling" rows are useless and actually degrade the user experience:
https://react-window.now.sh/#/examples/list/scrolling-indicators

Is my explanation clear enough? What do you think?
⇾ Is there a way to make the difference when rendering a row?

⇾ We wish to display 'is scrolling' rows only when user scrolls fast. If needed we are ready to use overscanCount prop and test performance on various devices to determine a satisfying threshold, but currently this prop has no effect on isScrolling value.
https://codesandbox.io/s/x3161x9voq

@bvaughn
Copy link
Owner

bvaughn commented Jan 29, 2019

We wish to display 'is scrolling' rows only when user scrolls fast.

"fast" is not something that react-window has any concept of.

If needed we are ready to use overscanCount prop and test performance on various devices to determine a satisfying threshold, but currently this prop has no effect on isScrolling value.

I don't know what you mean by this.

To reduce this I have set prop useIsScrolling and used isScrolling row renderer property, but it doesn't seem to take into account if rows have already been rendered.

You could use a custom areEquals function for this! Here's an example:
https://codesandbox.io/s/j2o88qy14v

This basically skips re-rendering your component if the only thing that changed from isScrolling false to true.

I think the default behavior of the library is correct though and this micro-optimization is reasonable to leave up to an application developer. Happy to continue chatting about this if you have more ideas though!

Going to close this issue for now since I don't think it's actionable as is.

@bvaughn bvaughn closed this as completed Jan 29, 2019
@bvaughn bvaughn added the 💬 question Further information is requested label Jan 29, 2019
@guirip
Copy link
Author

guirip commented Jan 30, 2019

Hello

Thank you for your fast answer (as usual).

I think that I misunderstood the role of isScrolling property. I thought it was a fallback, something triggered by react-window when the user is scrolling so fast that rendered element would be visible for a so short duration that rendering them feels useless.

1 - Here is the default behaviour we were trying to evince, see when the screen goes mostly white:
http://kim1.mobile-spot.com/react-window-videos/1-out-of-box-white-sections.mp4

2 - Here is what happens when using FixedSizeList's useIsScrolling prop to render blank rows:
http://kim1.mobile-spot.com/react-window-videos/2-out-of-the-box-isScrolling.gif
We definitely do not want that.

3 - Using your areEqual function, rendered rows are preserved, thanks!
http://kim1.mobile-spot.com/react-window-videos/3-custom-areEqual-isScrolling.gif
But you quickly meet blank rows due to isScrolling === true on new rows to render.

4 - We can reduce this by setting an overscanCount (value is 15 in the sequence below)
http://kim1.mobile-spot.com/react-window-videos/4-custom-areEqual-overscanCount15-isScrolling.gif
Pretty pointless as it just adds a delay.

I think these efforts were actually vain. Default behaviour (see 1) is the closest of what we expect. Screen going white is not something react-window can do anything about, it is due to the device hardware limitation and rows being a bit heavy to render. Correct?

Nevertheless after these various tests I don't really see a use case for useIsScrolling prop, I don't see how it can improve user experience.

Thank you for your time and the tips

@bvaughn
Copy link
Owner

bvaughn commented Jan 30, 2019

isScrolling is true or false based on whether the current render is in respond to a "scroll" event essentially. It's an optimization that isn't needed in most cases, so it's opt-in.

One thing I noticed about the videos– or at least what I guessed, based on the port number– is that it's running in DEV mode. DEV mode is definitely known to be slower (for React in general) for what it's worth. Measure the production build for more accurate sense of how things are rendering.

Looking at your rows, they don't look that "heavy" so I wouldn't expect that you'd need to use isScrolling. Is this code somewhere I can take a peek at?

@guirip
Copy link
Author

guirip commented Jan 30, 2019

Thank you for your kind offer, but I have the regret to say that we decided to stop the migration to react-window because it generates too much extra work. The simple list works great, but:

  • lists are used a lot in our app in various contexts (modals, accordions, nested components computing themselves their available height...) which involve extra work
  • we struggle to handle rows of variable height (some labels can wrap, and font changes from a project to another, we don't see how to determine in a bullet-proof way and at low cost how much height an item will need)
  • some weird bug on iOS (as usual, always a PITA) when overscrolling

Anyway thank you for your support! It is possible that one day we unearth that migration branch when we have more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants