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

Improve collection items scrolling #16738

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Sep 25, 2023

Requires #16725

We know the total number of items in a collection since they are immutable. We can then initialize the virtual scroller array with all the items as placeholders and then just fetch those that are missing real data. The direct benefit is that we can scroll and navigate to the end of the collection (or any point in the middle) much more smoothly.

This approach might be a bit naive though as with potentially huge collections we are creating a lot of items that will probably never be navigated to... but it is probably worth it.

We can observe the differences in scrolling behavior in the examples below with a modest collection containing 25k items.

Note: expanding a placeholder has been disabled.

Before

  • Scrolling to the bottom of the collection is time-consuming and almost "impossible" for large collections.
  • The scroll bar doesn't represent the actual size of the collection, so it keeps "popping" whenever new items get loaded.
CollectionScrollingBefore.mp4

After

  • You can smoothly scroll to any point of the collection within seconds.
  • The scrollbar matches the actual size of the collection, so moving it to the very bottom will take you directly to the bottom of the collection.
CollectionScrollingWithPlaceholders.mp4

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@davelopez davelopez force-pushed the explore_collection_item_scroll_improvements branch from e0f245c to 179eca8 Compare September 27, 2023 12:12
@davelopez davelopez force-pushed the explore_collection_item_scroll_improvements branch from 179eca8 to 958039d Compare September 27, 2023 16:01
@davelopez davelopez marked this pull request as ready for review September 27, 2023 16:03
@github-actions github-actions bot added this to the 23.2 milestone Sep 27, 2023
@davelopez davelopez marked this pull request as draft September 28, 2023 13:01
Since we know the expected total number of elements we can let the virtual scroller know it by creating the expected number of items beforehand and then update them as we fetch the details.
Remove unnecessary defaults and keep it to minimum.
- Make `expandDataset` optional with default to false.
- Reorder props to put required ones first.
@davelopez davelopez force-pushed the explore_collection_item_scroll_improvements branch from be295dc to 739193a Compare October 11, 2023 10:18
@davelopez davelopez changed the title Explore collection item scroll improvements Improve collection items scrolling Oct 11, 2023
@davelopez davelopez marked this pull request as ready for review October 11, 2023 12:52
@davelopez
Copy link
Contributor Author

This is finally ready for review.
I've updated the videos in the description using a collection with 25000 datasets and seems to work pretty smoothly on my local tests.

* This is used to determine the offset to fetch the element from when
* scrolling through the collection.
*/
element_index?: number;
Copy link
Member

Choose a reason for hiding this comment

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

Very minor: Don't you always know the element_index ? Similarly, could you require fetching to be a boolean and init it with true ?

Copy link
Contributor Author

@davelopez davelopez Oct 11, 2023

Choose a reason for hiding this comment

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

Good point, making element_index optional here was just to be able to make it work with Array.fill but now that I look at it we still need to map the index to each placeholder so making it required and using simple for loop would be even better. I'll go and do that 👍

About making the fetching boolean default to true... I'm not so sure, maybe default them to false, I think not even setting it by default will save a tiny bit of memory if you have a huge number of elements that you would never navigate to 😅

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Clever, I like it!

@bgruening
Copy link
Member

That works super smooth!

@bgruening bgruening merged commit cde7658 into galaxyproject:dev Oct 14, 2023
18 checks passed
@bgruening
Copy link
Member

Thanks @davelopez

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

Successfully merging this pull request may close these issues.

3 participants