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

Add virtualization support to data-grid #210

Closed
rchiodo opened this issue Sep 21, 2021 · 15 comments
Closed

Add virtualization support to data-grid #210

rchiodo opened this issue Sep 21, 2021 · 15 comments
Labels
enhancement New feature or request
Milestone

Comments

@rchiodo
Copy link

rchiodo commented Sep 21, 2021

Feature/component description

The current implementatio/docs for the https://github.com/microsoft/vscode-webview-ui-toolkit/blob/main/src/data-grid/README.md allows a caller to add rows through a property. This works great for a small number of rows but wouldn't work so well for say a million rows.

Use case

We'd love to use the data grid for this control here: https://code.visualstudio.com/docs/datascience/jupyter-notebooks#_variable-explorer-and-data-viewer

The variable explorer cannot fetch all rows at once so virtualization of row data is required.

The data frame viewer is even worse. We need to support data frames with millions of rows and columns.

@rchiodo rchiodo added the enhancement New feature or request label Sep 21, 2021
@daviddossett
Copy link
Collaborator

daviddossett commented Sep 23, 2021

@rchiodo what are you using for a data grid today? Is it custom built for the Python/Jupyter extensions?

@rchiodo
Copy link
Author

rchiodo commented Sep 23, 2021

We actually use a wrapper around slickGrid (which supports virtualizing columns and rows).

Here's our wrapper:
https://github.com/microsoft/vscode-jupyter/blob/main/src/datascience-ui/data-explorer/reactSlickGrid.tsx

I believe we use fluent-ui styles to make it look like VS code.

@chrisdholt
Copy link
Member

@daviddossett we're currently exploring virtualization in FAST a couple of ways, it would be great to get your thoughts/requirements/needs captured as part of that to enable support if needed. Ideally I'm hoping for a couple of approaches, one being a component and one being a class that can be used within components that have strict structural/semantic requirements (such as tree view, listbox, etc). microsoft/fast#5256

@scomea
Copy link

scomea commented Nov 16, 2021

Interesting. I've got the virtualizing data-grid prototype linked above working to the point where keyboarding to a currently virtualized row works (mostly). Feedback appreciated btw.

A few questions -

  • column virtualization: The current fast data-grid already allows authors to control what columns are rendered out of the provided data set so you can easily add/remove/reorder them, or do you literally mean a horizontally scrollable million-column grid? We should be able to virtualize out of viewport cells in each row using the same approach, but I haven't thought of that as an early priority.
  • "millions of rows": There are upper limits to how big elements can get before things get janky and I haven't looked at those issues in detail yet. Scrolling/painting gets weird in my test grid after about a half a million rows.

@hawkticehurst
Copy link
Member

^ cc @rchiodo

@rchiodo
Copy link
Author

rchiodo commented Nov 16, 2021

do you literally mean a horizontally scrollable million-column grid?

Yes this one. Features in data science can be on the order of millions. At least for really complex stuff.

"millions of rows": There are upper limits to how big elements can get before things get janky

Yes millions of rows. This is a common case (more so than the columns, columns are usually in the 1000s).

I believe the grid we're using now has a canvas that it uses to render the current visible rows (and columns) (so it never has more than 3 pages I think in memory).

@hawkticehurst
Copy link
Member

@scomea I'm also curious to know what the API will look for using virtualization and if it will be compatible with fast-react-wrapper?

I ask because a lot of our community and (if I recall correctly) even the VS Code Jupyter extension that @rchiodo works on uses React.

So, if at all possible, I'd love if the React integration story is considered as part of this work too (and of course I'm more than happy to help out with feedback or any other way that I can).

@scomea
Copy link

scomea commented Nov 16, 2021

I believe the grid we're using now has a canvas that it uses to render the current visible rows (and columns) (so it never has more than 3 pages I think in memory).

@rchiodo When you say "in memory" do you mean how many rows have associated elements rendered in the DOM, or something else?

@chrisdholt
Copy link
Member

@scomea I'm also curious to know what the API will look for using virtualization and if it will be compatible with fast-react-wrapper?

I ask because a lot of our community and (if I recall correctly) even the VS Code Jupyter extension that @rchiodo works on uses React.

So, if at all possible, I'd love if the React integration story is considered as part of this work too (and of course I'm more than happy to help out with feedback or any other way that I can).

IMO, the react work should be transparent here. The wrapper serves to address problems which are caused by React's lack of compatibility with the DOM standard and I don't think we should hoist that requirement to a web component which, upon support being introduced, would be unnecessary. With that said, I don't see any implementation which would require us to provide special handling for react beyond the wrapper; hopefully it's a non-issue. Just want to provide a heads up on this though, so it's not a surprise...

@rchiodo
Copy link
Author

rchiodo commented Nov 16, 2021

@rchiodo When you say "in memory" do you mean how many rows have associated elements rendered in the DOM, or something else?

I may be wrong, but the canvas I think is hosting the 3 pages of elements that are visible. The source code for the grid we use is here: https://github.com/6pac/SlickGrid

@hawkticehurst
Copy link
Member

IMO, the react work should be transparent here. The wrapper serves to address problems which are caused by React's lack of compatibility with the DOM standard and I don't think we should hoist that requirement to a web component which, upon support being introduced, would be unnecessary. With that said, I don't see any implementation which would require us to provide special handling for react beyond the wrapper; hopefully it's a non-issue. Just want to provide a heads up on this though, so it's not a surprise...

Oh yeah sorry if I wasn't as clear, I completely agreed that the web components should not shoulder/take on React's lack of compliance with the DOM standard. My question was indeed just supposed to make sure that there weren't any gotchas or special handling that would be required beyond wrapping the component.

But it sounds like it shouldn't be an issue so I'm happy. 🙂 Thanks for clarifying @chrisdholt!

@connor4312
Copy link
Member

Something else we just hit in the hex editor: very large scroll regions will reach the maximum scroll height defined in browsers, and therefore we had to implement non-native scrolling. May or may not be in scope for this, but it sounds like something the Python team may hit. The cap is somewhere around 10 million pixels, I didn't test in great depth.

@scomea
Copy link

scomea commented Nov 16, 2021

@scomea I'm also curious to know what the API will look for using virtualization

For a basic scenario all that should be required is for the author to set the "virtualize" flag and provide item height. That should result in a navigable virtualized grid. Once authors want to be able to move focus to virtualized elements and the such that gets more complicated. This really is the ideal stage for hammering out the api.

@scomea
Copy link

scomea commented Nov 16, 2021

Something else we just hit in the hex editor: very large scroll regions will reach the maximum scroll height defined in browsers, and therefore we had to implement non-native scrolling. May or may not be in scope for this, but it sounds like something the Python team may hit. The cap is somewhere around 10 million pixels, I didn't test in great depth.

Yeah, I've been thinking we'll need to do some special scrolling implementations at some point to support very large displays.

@hawkticehurst
Copy link
Member

Hi all,

Back with a final update: I'm very sorry to say that the toolkit is being deprecated and all active development will be coming to a close.

There was an announcement last week where you can learn more details and leave any questions or comments you may have.

Beyond that, thank you so much filing this issue and apologies for never getting around to addressing it. It means a lot that you contributed to the improvement of this project. I wish you all the best in your future VS Code extension endeavors!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants