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

Use URLs directly for layout images #7199

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Conversation

alexcjohnson
Copy link
Collaborator

The data uri is necessary for export only, which should always use staticPlot - so if we're not in staticPlot mode, skip this conversion. Hoping this will improve the experience for plots that make heavy & dynamic use of layout images, by keeping the plotting process synchronous - which I'm calling a bug because we've seen the display become glitchy during pan/zoom in these cases.

I'll have a couple of tests to adapt as well as a changelog entry to add... but first I'm going to use the circleci build for testing in our application.

The data uri is necessary for export only, which should always use staticPlot
@alexcjohnson alexcjohnson added bug something broken performance something is slow labels Oct 2, 2024
@archmoj archmoj marked this pull request as ready for review October 2, 2024 20:32
@archmoj
Copy link
Contributor

archmoj commented Oct 2, 2024

@alexcjohnson Thanks for the PR. LGTM.
Please add a draftlog.
Also possibly investigate these failures: https://app.circleci.com/pipelines/github/plotly/plotly.js/11406/workflows/ea736858-eed0-4644-bbac-f80d66624105/jobs/252337/parallel-runs/9/steps/9-105

Concerning the image test failures we did address the problem on master in #7178; so you could skip them.

@archmoj
Copy link
Contributor

archmoj commented Oct 3, 2024

Please fetch upstream/release-v2.35.3 branch and merge it into this branch.
That would fix the image tests.
Thank you!

@alexcjohnson
Copy link
Collaborator Author

@archmoj one of the tests that fails with my change is should remove the image tag if an invalid source. Instead we see the browser's "broken image" icon if it fails to load. I think this is useful behavior for chart creators so I'm inclined to leave it this way and adapt the test (run as-is in staticPlot mode but verify that the image points to the source unchanged in interactive mode), do you agree or do you want to call this a breaking change that I should fix?

Here's me intentionally breaking the first image of the layout-images mock:
Screenshot 2024-10-07 at 16 06 18

FYI we've demonstrated that this greatly improves the experience for our tile-server-in-a-graph app - using relayout data to decide which images to show, so overall it's definitely a win!

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

💃

@archmoj archmoj merged commit c3454a5 into release-v2.35.3 Oct 8, 2024
1 check passed
@archmoj archmoj deleted the alex/direct-images branch October 8, 2024 13:15
@archmoj archmoj restored the alex/direct-images branch December 12, 2024 20:16
@archmoj archmoj deleted the alex/direct-images branch December 12, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken performance something is slow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants