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.

Sign up for GitHub

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

Jump to bottom

Fix: Render/resize the plot only if it is visible #1029

Closed
wants to merge 3 commits into from
Closed

Fix: Render/resize the plot only if it is visible #1029

wants to merge 3 commits into from

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Jan 20, 2020 •
edited
Loading

This delays the rendering and the resize of the plot to when the plot is actually visible on the page.

It is using the IntersectionObserver API for knowing if the plot is visible on the page or not

This fixes an issue when having bqplots in Tab widgets.

Fixes #1023

This is related to jupyter-widgets/ipywidgets#2605

@martinRenou
Copy link
Member Author

Opening this PR as draft. We really need to discuss this.

All of this might need a change in ipywidgets: wait for the first after-show event to be triggered before resolving the displayed promise.

cc. @maartenbreddels

@martinRenou
Render the plot on first after-show event
0c56d3b 
This delays the rendering of the plot to when the plot is actually
visible on the page and has available space.

This fixes an issue when having bqplots in Tab widgets.

Because we waited for the displayed promise to be resolved (which
is resolved on after-attach), the plot was rendered under invisible
tabs with an available size of 0, and was not resized when clicking
on the tab. This was resulting in a weirdly shrinked plot.

This changes this behavior by waiting for the element to be actually
shown on the page (after-show is triggered when clicking on the tab) and
has available space.

Signed-off-by: martinRenou <[email protected]>
@martinRenou martinRenou marked this pull request as ready for review January 21, 2020 12:20
@martinRenou
Copy link
Member Author

This will fix #1023 by lazily rendering the plot.

lazy_load

@martinRenou
Copy link
Member Author

martinRenou commented Jan 21, 2020 •
edited
Loading

Unfortunately, it does not work well when doing a resize of the page

resize

@martinRenou martinRenou changed the title Render the plot on first after-show event WIP - Render the plot on first after-show event Jan 21, 2020
@martinRenou
Use the intersectionObserver
2ae204a 
Signed-off-by: martinRenou <[email protected]>
@martinRenou
Copy link
Member Author

@maartenbreddels I have a working version using the intersection observer. Although it breaks tests for now.

@martinRenou martinRenou changed the title WIP - Render the plot on first after-show event Fix: Render the plot on first after-show event Jan 22, 2020
@martinRenou martinRenou changed the title Fix: Render the plot on first after-show event Fix: Render/resize the plot only if it is visible Jan 22, 2020
@martinRenou
Fix tests
493f13d 
Signed-off-by: martinRenou <[email protected]>
@martinRenou
Copy link
Member Author

Tests are passing again by enforcing the visible status of the plot in tests.

@martinRenou
Copy link
Member Author

@maartenbreddels I'd be interested to know if that works with your tablet

@martinRenou martinRenou mentioned this pull request Jan 28, 2020
Closed
maartenbreddels
maartenbreddels reviewed Jan 28, 2020
View reviewed changes
Copy link
Member

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Looks good, I did not test on my tablet (not here). It does add a lot of complexity, but I'm not sure we can avoid that.

@jasongrout
Copy link
Member

I've also opened a new issue about fixing the underlying issue in ipywidgets: jupyter-widgets/ipywidgets#2762

@jasongrout jasongrout mentioned this pull request Jan 28, 2020
Closed
@jasongrout
Copy link
Member

You could just listen for the after-attach and after-show Lumino events, right?

@martinRenou
Copy link
Member Author

It was actually the first version of this PR (see 0c56d3b), but it was not working properly (see this comment).

Also, the intersection Observer is a bit different from the after-show. It will trigger the show event only if the plot is actually visible on the screen (some pixels are visible). While the Lumino's after-show event is triggered even if the page is completely scrolled down and the plot is not visible.
So it also brings speed improvements on the resize: only resizing plots that are visible.

@jasongrout
Copy link
Member

So it also brings speed improvements on the resize: only resizing plots that are visible.

How does this handle the case where you scroll a visible bqplot plot off the screen, then resize the browser window, then scroll back to see the plot? Will it automatically resize as soon as it becomes visible?

@jasongrout jasongrout mentioned this pull request Mar 4, 2020
Merged
@jasongrout
Copy link
Member

#1047 is an alternative implementation that uses the phosphor after-show event. It's not as fancy, in that it doesn't use the browser visibility capabilities, but it is quite a bit simpler. Thoughts?

@martinRenou
Copy link
Member Author

How does this handle the case where you scroll a visible bqplot plot off the screen, then resize the browser window, then scroll back to see the plot? Will it automatically resize as soon as it becomes visible?

Yes :)

@martinRenou
Copy link
Member Author

#1047 is an alternative implementation that uses the phosphor after-show event. It's not as fancy, in that it doesn't use the browser visibility capabilities, but it is quite a bit simpler. Thoughts?

I agree. I tried this approach at first but could not make it work. I guess I forgot the this.pWidget.isVisible part that I did not know of.

Even though this PR brings more optimizations on the resize (resize plot only if it is visible), I would say simpler is better, we can close my PR.

@martinRenou
Copy link
Member Author

Closed in favor of #1047

@martinRenou martinRenou closed this Mar 4, 2020
@martinRenou martinRenou deleted the render_on_first_show branch March 4, 2020 16:03
@kaiayoung kaiayoung mentioned this pull request Mar 13, 2020
Closed
@mariobuikhuizen mariobuikhuizen mentioned this pull request Oct 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maartenbreddels maartenbreddels maartenbreddels left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem displaying bqplot within an ipywidget tabbed interface
3 participants
@martinRenou @jasongrout @maartenbreddels