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

Problem Exporting Charts as Image #29614

Closed
wants to merge 1 commit into from

Conversation

xiaoqufengdi
Copy link

It is wrong not to display a chart if it is not in the browser window display range. When we download the dashboard as a picture, we find that some charts are not displayed

#28713

fix(dashboard): load charts correctly

It is wrong not to display a chart if it is not in the browser window display range. When we download the dashboard as a picture, we find that some charts are not displayed

apache#28713
@dosubot dosubot bot added the dashboard:export Related to exporting dashboards label Jul 17, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@rusackas rusackas added the review:checkpoint Last PR reviewed during the daily review standup label Jul 17, 2024
@rusackas
Copy link
Member

This PR disables a feature that was implemented to improve performance. We don't render all charts on a dashboard, but rather just render them when they're in the viewport. Dashboards with hundreds of charts will have poor performance with this change.

@kgabryje do you know of any way to make this change dynamic, so screenshots/alerts/reports/thumbnails/etc will work as expected? There are several GitHub issues about this, I'll try to find/link them here.

@xiaoqufengdi
Copy link
Author

This PR disables a feature that was implemented to improve performance. We don't render all charts on a dashboard, but rather just render them when they're in the viewport. Dashboards with hundreds of charts will have poor performance with this change.

@kgabryje do you know of any way to make this change dynamic, so screenshots/alerts/reports/thumbnails/etc will work as expected? There are several GitHub issues about this, I'll try to find/link them here.

I understand the optimization, but you shouldn't hide it after the chart has been shown once.

@michael-s-molina michael-s-molina removed the review:checkpoint Last PR reviewed during the daily review standup label Jul 18, 2024
@kgabryje
Copy link
Member

kgabryje commented Jul 19, 2024

@xiaoqufengdi Thanks for reporting! PR #29272 that was merged yesterday should fix the issue. Could you please verify that it works for you now?
CC @rusackas @geido

@geido
Copy link
Member

geido commented Jul 30, 2024

I am closing this for now as the solution won't be merged as is. Please, feel free to verify that the new changes introduced in #29272 have fixed the issue or feel free to re-open the PR with an appropriate solution. Thanks for your help!

@geido geido closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dashboard:export Related to exporting dashboards size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants