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

feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard #25696

Merged
merged 15 commits into from
Oct 31, 2023

Conversation

fisjac
Copy link
Contributor

@fisjac fisjac commented Oct 18, 2023

SUMMARY

This PR changes the UI on the dashboard page to include an export as pdf button to the rightmost dropdown menu of the header on the dashboard page. This download functionality takes rasterized screenshots of each individual chart component and joins them into a downloadable pdf printout of the dashboard.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before menu:
image

After menu:
image

After download functionality:
https://www.loom.com/share/a8c39c1b81f04226beb5fa2a2d24010a?sid=87eed984-97db-4016-8ff9-32ad8b6b39e2

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@fisjac fisjac changed the title feature:Dom to pdf feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard Oct 18, 2023
@fisjac
Copy link
Contributor Author

fisjac commented Oct 18, 2023

Adding initial functionality for rasterized screenshot pdfs inspired in large part by @Yaswanth-Perumalla in PR #25460

Additional improvements to the pdf function will be needed to move it away from static screenshots.

@john-bodley
Copy link
Member

john-bodley commented Oct 19, 2023

Thanks @fisjac for the PR. Would you mind completing the PR description or switch to draft mode if this is still WIP.

Maybe there should be a discussion about whether we should use the the native browser print to PDF option.

@rusackas
Copy link
Member

If the PDF is rasterized, does it have any advantage over the PNG export? The main reason to export PDF would seem to be to support text and vectors.

@fisjac fisjac marked this pull request as draft October 19, 2023 16:56
@fisjac fisjac marked this pull request as ready for review October 23, 2023 19:54
@eschutho
Copy link
Member

eschutho commented Oct 24, 2023

If the PDF is rasterized, does it have any advantage over the PNG export? The main reason to export PDF would seem to be to support text and vectors.

@rusackas Agree, Ideally we'll move toward a vector PDF, but since we don't have great responsive css, it all just looks broken when we try to resize the window to an A11 sheet of paper size. Same with using the built in print to PDF functionality in the browser @john-bodley. It was much easier to take a screenshot image and then resize the image. Definitely an MVP for a larger feature in the near future to have a vector PDF once we can tackle the responsive CSS functionality.

Happy to hear any other thoughts as to whether this interim functionality is a good short-term solution.

Comment on lines +66 to +69
.then(() => {
// nothing to be done
})
.catch((e: Error) => {
Copy link
Member

@hughhhh hughhhh Oct 27, 2023

Choose a reason for hiding this comment

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

nit: I think it would be good to do success toast here to let the user know that download was successful. Just search examples of successToast in our codebase + you already have it imported above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hughhhh I was following the same flow for other downloads throughout superset. Since there are no successToasts in the download flow when downloading images from dashboard and explore tabs, does it still make sense to add this? If so, we should probably add those consistently to the other download buttons.

@yousoph
Copy link
Member

yousoph commented Oct 27, 2023

/testenv up

@github-actions
Copy link
Contributor

@yousoph Ephemeral environment spinning up at http://34.220.83.24:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@yousoph
Copy link
Member

yousoph commented Oct 27, 2023

@fisjac on the test environment ^ I was able to get the Video Game Sales dashboard as a PDF, but not the Slack Dashboard. Slack Dashboard worked properly when downloading as image.

@fisjac
Copy link
Contributor Author

fisjac commented Oct 28, 2023

@yousoph I just tried the slack dashboard in the test environment. It worked for me, but took a bit longer than Video Game Sales.

@hughhhh hughhhh merged commit 74dbada into apache:master Oct 31, 2023
26 checks passed
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@giftig
Copy link
Contributor

giftig commented Jan 8, 2024

Not sure if this is directly related, but I've seen frontend build errors while running the project in docker-compose with the latest master:

ERROR in ./node_modules/jspdf/dist/jspdf.es.min.js 128:659-680
Module not found: Error: Can't resolve 'html2canvas' in '/app/superset-frontend/node_modules/jspdf/dist'

ERROR in ./node_modules/jspdf/dist/jspdf.es.min.js 291:65-80
Module not found: Error: Can't resolve 'canvg' in '/app/superset-frontend/node_modules/jspdf/dist'

and it looks like jspdf is used by dom-to-pdf, which was first used in this change. @fisjac maybe you'll have a better idea what might be happening here than me, I'm not very familiar with these packages.

@fisjac
Copy link
Contributor Author

fisjac commented Jan 17, 2024

@giftig, I'll create a separate bug ticket for this. It looks like this is being caused due to html2canvas and canvg being installed as optional dependencies within the jspdf package. For a local fix, moving those packages from optionalDependencies to dependencies in the jspdf section of the package-lock.json file should address the errors.

@rusackas
Copy link
Member

I haven't opened a PR ye, but it seems like this is resolved if you drop dom-to-pdf from version 0.3.2 to 0.3.1.

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants