-
Notifications
You must be signed in to change notification settings - Fork 12
Updated virtualization logic to avoid re-rendering #1075
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
base: main
Are you sure you want to change the base?
Updated virtualization logic to avoid re-rendering #1075
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all a nice improvement as it removes a significant amount of code and stabilizes the component.
A couple of comments (general, not only related to your changes):
- Usually, when having a controlled component, you would not want
onChangecallbacks to trigger when you provide new data (hence the programmatic check before). However, as the scroll event is actually a DOM event, I think it's okay to mirror it (as long as we document it clearly enough). I checked the issues which had required this implementation back then and it's no longer causing issues firing the event also on programmatic changes. So we are good with your change. However, we should keep an eye on it just in case. Alternatively, we could also add an optional prop for controlling the behaviour. - You removed the tail-overscan (rendering one more item than visible at the end). I think it is good to keep the overscan as it improves the UX by avoiding placeholder pop-ins on scroll. We could add a prop
overscan/overscanCountfor symmetric overscan (head & tail) that defaults to 1. - Two things I would change in the
mountScrollEffect- Assign
props.containerRef.currentto a local variable inside the effect and use that variable in the unmount return function.ref.currentcan change in between and would cause the event listener to never be removed. Also, you can do the assignment at the very beginning and add an early return whennulldirectly after - this way we can remove the later checks. - Replace
props.itemswithprops.items.length- we do not really care about the individual items, only about the size of the array.
- Assign
- We should add a guard for
props.itemSizeto guarantee > 0 (see division byitemSize). - I think we don't really need to use
lodash.isEqualto compare the ranges - a more native check is sufficient - but either is fine as we're usinglodashall over the place.
|
rubenthoms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good! Some additional improvements to consider.
rubenthoms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discovered one tiny issue with the end index calculation. The end index should be inclusive the last (halfway) visible element.
Now refers to only the visible items. Overscan is now only applied when needed (placeholder sizes and rendered items)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all very good. Some nitpicks. There is also some issue with the Select component now that is probably related to the changes in Virtualization. To reproduce you can select a couple of ensembles, e.g. 8, and add a Simulation time series module. Then select an ensemble at the bottom (without scrolling) - the select content is automatically scrolling now (see video). This should definitely be addressed.
Updates the
Virtualizationcomponent:onScrollgive the end-index aswellonScrollhandler wasn't stableprops.startIndexeverytime the data list updates)props.startIndexchanges, the component will scroll to it's value