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

Switch to overlay webviews for notebooks #5829

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

nstrayer
Copy link
Contributor

Addresses #5491

This PR changes how we render html output in notebooks 2.0 by switching to overlay webviews instead of standard ones.
The reason for this switch was that there is no way to preserve the content of the webview across visibility changes with standard webviews. This means we would have to fully re-render every output every time the user switched to the notebook view.

One benefit of this switch is we were able to remove all the logic surrounding different webview types being returned from the various webview services, simplifying the API a good bit.

QA Notes

  • Dynamic plots like hvplot should still show up as expected.
  • You should be able to navigate away from the notebook and back and the plot should reappear in the same state it was in.
  • Plots should never spill out of the notebook containment. (e.g. be visible when the notebook isnt. This is an annoying problem with overlay webviews that we've had to deal with before in the plots pane etc..)

- Added NotebookVisibilityProvider to manage editor visibility state.
- Introduced observable for tracking editor visibility in PositronNotebookEditor.
- Updated setEditorVisible method to set visibility state.
- Modified webview mounting logic to conditionally mount based on visibility.
@nstrayer nstrayer requested a review from seeM December 19, 2024 14:03
Copy link

github-actions bot commented Dec 19, 2024

E2E Tests 🚀  ?
This PR will run tests tagged with: @critical

Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

I checked out the branch and played around with it a bit and found a few issues.

In a notebook with two cells:

import hvplot.pandas
import pandas as pd
import plotly.express as px
df = px.data.iris()
fig = px.scatter(df, x='sepal_width', y='sepal_length')
fig
  1. The plot seems to be placed over the cell selection border, see the section in the red box:

  2. The plot remains if I switch between two notebooks:

    Screen.Recording.2024-12-19.at.16.59.25.mov
  3. There's a similar issue when showing/hiding the bottom pane with Cmd+J:

    Screen.Recording.2024-12-19.at.17.00.58.mov

@ntluong95
Copy link

Currently this experimental notebook doesn't have running animation as well as cell execution completion info. Hope these features will come soon

@nstrayer
Copy link
Contributor Author

@ntluong95 , see #5495 and there is a simple running animation (a pulsing border around the cell executing).

Feel free to make a new issue for the cell execution animation with ideas about it - it's a tricky problem and we'd love your input! Also reacting to the other issue can help us prioritize! Currently we're focusing on the underpinnings of the current notebooks to get them rock solid before making a big push on these experimental notebooks.

@nstrayer nstrayer requested a review from seeM December 20, 2024 15:07
@nstrayer
Copy link
Contributor Author

@seeM I couldn't reproduce 1 locally but the others should be fixed. Let me know if you still have the problem with overlap.

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

Successfully merging this pull request may close these issues.

3 participants