You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "fisjac (via GitHub)" <gi...@apache.org> on 2023/10/18 22:52:50 UTC
[PR] feature:Dom to pdf [superset]
fisjac opened a new pull request, #25696:
URL: https://github.com/apache/superset/pull/25696
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [x] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1783591562
@yousoph Ephemeral environment spinning up at http://34.220.83.24:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #25696:
URL: https://github.com/apache/superset/pull/25696#discussion_r1374790116
##########
superset-frontend/src/utils/downloadAsPdf.ts:
##########
@@ -0,0 +1,73 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SyntheticEvent } from 'react';
+import domToPdf from 'dom-to-pdf';
+import kebabCase from 'lodash/kebabCase';
+import { logging, t } from '@superset-ui/core';
+import { addWarningToast } from 'src/components/MessageToasts/actions';
+
+/**
+ * generate a consistent file stem from a description and date
+ *
+ * @param description title or description of content of file
+ * @param date date when file was generated
+ */
+const generateFileStem = (description: string, date = new Date()) =>
+ `${kebabCase(description)}-${date.toISOString().replace(/[: ]/g, '-')}`;
+
+/**
+ * Create an event handler for turning an element into an image
+ *
+ * @param selector css selector of the parent element which should be turned into image
+ * @param description name or a short description of what is being printed.
+ * Value will be normalized, and a date as well as a file extension will be added.
+ * @param isExactSelector if false, searches for the closest ancestor that matches selector.
+ * @returns event handler
+ */
+export default function downloadAsPdf(
+ selector: string,
+ description: string,
+ isExactSelector = false,
+) {
+ return (event: SyntheticEvent) => {
+ const elementToPrint = isExactSelector
+ ? document.querySelector(selector)
+ : event.currentTarget.closest(selector);
+
+ if (!elementToPrint) {
+ return addWarningToast(
+ t('PDF download failed, please refresh and try again.'),
+ );
+ }
+
+ const options = {
+ margin: 10,
+ filename: `${generateFileStem(description)}.pdf`,
+ image: { type: 'jpeg', quality: 1 },
+ html2canvas: { scale: 2 },
+ };
+ return domToPdf(elementToPrint, options)
+ .then(() => {
+ // nothing to be done
+ })
+ .catch((e: Error) => {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1901252528
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh merged PR #25696:
URL: https://github.com/apache/superset/pull/25696
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #25696:
URL: https://github.com/apache/superset/pull/25696#discussion_r1374790116
##########
superset-frontend/src/utils/downloadAsPdf.ts:
##########
@@ -0,0 +1,73 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SyntheticEvent } from 'react';
+import domToPdf from 'dom-to-pdf';
+import kebabCase from 'lodash/kebabCase';
+import { logging, t } from '@superset-ui/core';
+import { addWarningToast } from 'src/components/MessageToasts/actions';
+
+/**
+ * generate a consistent file stem from a description and date
+ *
+ * @param description title or description of content of file
+ * @param date date when file was generated
+ */
+const generateFileStem = (description: string, date = new Date()) =>
+ `${kebabCase(description)}-${date.toISOString().replace(/[: ]/g, '-')}`;
+
+/**
+ * Create an event handler for turning an element into an image
+ *
+ * @param selector css selector of the parent element which should be turned into image
+ * @param description name or a short description of what is being printed.
+ * Value will be normalized, and a date as well as a file extension will be added.
+ * @param isExactSelector if false, searches for the closest ancestor that matches selector.
+ * @returns event handler
+ */
+export default function downloadAsPdf(
+ selector: string,
+ description: string,
+ isExactSelector = false,
+) {
+ return (event: SyntheticEvent) => {
+ const elementToPrint = isExactSelector
+ ? document.querySelector(selector)
+ : event.currentTarget.closest(selector);
+
+ if (!elementToPrint) {
+ return addWarningToast(
+ t('PDF download failed, please refresh and try again.'),
+ );
+ }
+
+ const options = {
+ margin: 10,
+ filename: `${generateFileStem(description)}.pdf`,
+ image: { type: 'jpeg', quality: 1 },
+ html2canvas: { scale: 2 },
+ };
+ return domToPdf(elementToPrint, options)
+ .then(() => {
+ // nothing to be done
+ })
+ .catch((e: Error) => {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "fisjac (via GitHub)" <gi...@apache.org>.
fisjac commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1896689297
@giftig, this looks like a separate ticket should be opened 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1778224962
> 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.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1771373785
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1783621089
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "fisjac (via GitHub)" <gi...@apache.org>.
fisjac commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1783623815
@yousoph I just tried the slack dashboard in the test environment. It worked for me, but took a bit longer than Video Game Sales.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "fisjac (via GitHub)" <gi...@apache.org>.
fisjac commented on code in PR #25696:
URL: https://github.com/apache/superset/pull/25696#discussion_r1374801589
##########
superset-frontend/src/utils/downloadAsPdf.ts:
##########
@@ -0,0 +1,73 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SyntheticEvent } from 'react';
+import domToPdf from 'dom-to-pdf';
+import kebabCase from 'lodash/kebabCase';
+import { logging, t } from '@superset-ui/core';
+import { addWarningToast } from 'src/components/MessageToasts/actions';
+
+/**
+ * generate a consistent file stem from a description and date
+ *
+ * @param description title or description of content of file
+ * @param date date when file was generated
+ */
+const generateFileStem = (description: string, date = new Date()) =>
+ `${kebabCase(description)}-${date.toISOString().replace(/[: ]/g, '-')}`;
+
+/**
+ * Create an event handler for turning an element into an image
+ *
+ * @param selector css selector of the parent element which should be turned into image
+ * @param description name or a short description of what is being printed.
+ * Value will be normalized, and a date as well as a file extension will be added.
+ * @param isExactSelector if false, searches for the closest ancestor that matches selector.
+ * @returns event handler
+ */
+export default function downloadAsPdf(
+ selector: string,
+ description: string,
+ isExactSelector = false,
+) {
+ return (event: SyntheticEvent) => {
+ const elementToPrint = isExactSelector
+ ? document.querySelector(selector)
+ : event.currentTarget.closest(selector);
+
+ if (!elementToPrint) {
+ return addWarningToast(
+ t('PDF download failed, please refresh and try again.'),
+ );
+ }
+
+ const options = {
+ margin: 10,
+ filename: `${generateFileStem(description)}.pdf`,
+ image: { type: 'jpeg', quality: 1 },
+ html2canvas: { scale: 2 },
+ };
+ return domToPdf(elementToPrint, options)
+ .then(() => {
+ // nothing to be done
+ })
+ .catch((e: Error) => {
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1786313246
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "giftig (via GitHub)" <gi...@apache.org>.
giftig commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1880695785
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1771366136
Thanks @fisjac for the PR. Would you mind completing the PR description or switch to draft mode if this is still WIP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "fisjac (via GitHub)" <gi...@apache.org>.
fisjac commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1769528841
Adding initial functionality for rasterized screenshot pdfs inspired in large part by @Yaswanth-Perumalla in PR https://github.com/apache/superset/pull/25460
Additional improvements to the pdf function will be needed to move it away from static screenshots.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(Export as PDF - rasterized): Adding rasterized pdf functionality to dashboard [superset]
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #25696:
URL: https://github.com/apache/superset/pull/25696#issuecomment-1783590067
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org