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

Make the multiview template size dynamic for each plot inside #3761

Closed
wants to merge 2 commits into from

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Apr 25, 2023

Prototype for #3757

Screen.Recording.2023-04-25.at.4.20.00.PM.mov

This might actually work. This is better than I thought it would, probably because we already parse the template anyway at that time and calculation is straight forward as it had almost everything it needed.

There are some adjustments left, but this is just a prototype, I'll fix that in a secondary step. I first need to check if having multiple multi view plots would make this look laggy.

The one thing I don't like is the duplication of aspect ratios in the webview and the extension. If we change it some place, we'll forget to change at the other place. I'll try to make it so it's only in one place.

@sroy3 sroy3 added the product PR that affects product label Apr 25, 2023
@sroy3 sroy3 self-assigned this Apr 25, 2023
@sroy3
Copy link
Contributor Author

sroy3 commented Apr 26, 2023

Screen.Recording.2023-04-26.at.9.48.50.AM.mov

I'll need to add some debounce to the call to make this actually useful. Else it's going to move way too much.

@sroy3
Copy link
Contributor Author

sroy3 commented May 1, 2023

I don't think this is going to work after all. The problem is that when changing the template, we force the re-rendering of the React component. This will result in something very laggy, the more plots we have (especially with virtualized plots). The problem really is inside VegaLite and our templates were never the problem. To fix this we need to fix it after VegaLite has created the component or fix it inside VegaLite directly.

See video (too big for GitHub)

@sroy3 sroy3 closed this May 1, 2023
@shcheklein
Copy link
Member

The problem is that when changing the template, we force the re-rendering of the React component. This will result in something very laggy, the more plots we have (especially with virtualized plots).

It's still better vs what we have now (not responding at all to resize). Even if it takes substantial amount of time to re-render. If that happens only when we change size- I think it's better to have something then.

The problem really is inside VegaLite and our templates were never the proble

Let's create a ticket on their end to discuss it? How would you expect it to handle this? I think it would be great to describe / scope it.

@sroy3
Copy link
Contributor Author

sroy3 commented May 2, 2023

It's still better vs what we have now (not responding at all to resize). Even if it takes substantial amount of time to re-render. If that happens only when we change size- I think it's better to have something then.

It. might be hard to see if you're not personally experiencing it, it'll resize with a delay and block everything. Worse is when the plots are virtualized, they'll resize on scroll also. To me, it does not feel like a great experience at all. I made a separate fix last week that is also seen in this PR (#3774), which probably helps in making this look better.

Let's create a ticket on their end to discuss it? How would you expect it to handle this? I think it would be great to describe / scope it.

I'll do this.

@shcheklein
Copy link
Member

To me, it does not feel like a great experience at all.

Yep. The way you are describing it it's not good indeed. What I struggle to understand why it's not possible to implement it with changing the template on the resize w/o these delays, etc, etc.

I made a separate fix last week that is also seen in this PR (#3774), which probably helps in making this look better.

tbh, not sure I understand how the fix is related. Could you explain it please?

@sroy3
Copy link
Contributor Author

sroy3 commented May 2, 2023

To me, it does not feel like a great experience at all.

Yep. The way you are describing it it's not good indeed. What I struggle to understand why it's not possible to implement it with changing the template on the resize w/o these delays, etc, etc.

There are a few reasons for that:

  • First, there is a lot of data inside of a single plot. Every time the template changes, we need to re-render it in React. The more plots there are, the slower it's going to get because React has to reconcile every prop on each plot. Large complex data is slow in general in React if changed. Every performance optimization the we added to plot is bypassed to resize at the template level while adding a huge calculation at the end. (https://react.dev/learn/render-and-commit#optimizing-performance -> every node inside that SVG is a nested component)
  • Virtualized plots will only render once in view (or close to view), which means we ask for its size to be calculate once we scroll to that plot, causing a jump in the layout.
  • Also because we decide to change the size of the plot upstream, when we change the width or height, we have to send this back to the extension side where we calculate what the plot size will be, parse the template data to include this inside its spec and return this to the webview. Each round trip adds a little delay compared to other plots.

I made a separate fix last week that is also seen in this PR (#3774), which probably helps in making this look better.

tbh, not sure I understand how the fix is related. Could you explain it please?

It definitely does not solve everything, but if a plot acted as if it had 3 revisions inside of it, while it actually had only one, it would actually take as much room as 3 plots instead of 1, making the plot smaller in some cases.

@mattseddon mattseddon deleted the dynamic-template-size branch August 1, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants