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

move download chart PDF function to the front-end #2094

Merged
merged 3 commits into from
Mar 25, 2019
Merged

move download chart PDF function to the front-end #2094

merged 3 commits into from
Mar 25, 2019

Conversation

dippindots
Copy link
Member

Create SVG to PDF using the library: svg2pdf.js
Fix cBioPortal/cbioportal#5738.

@dippindots dippindots requested a review from alisman March 6, 2019 19:07
@dippindots dippindots self-assigned this Mar 6, 2019
alisman
alisman reviewed Mar 8, 2019
View reviewed changes
src/pages/staticPages/tools/oncoprinter/Oncoprinter.tsx
if (!svgToPdfDownload("oncoprint.pdf", this.oncoprint.toSVG(true))) {
alert("Oncoprint too big to download as PDF - please download as SVG.");
}
svgToPdfDownload("oncoprint.pdf", this.oncoprint.toSVG(false));
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe wrap this in try catch and notify user if there's an error

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I can do that. But should we have a limitation on the file size of SVG? For a very large oncoprint SVG, it may take half a minute for downloading.

alisman
alisman reviewed Mar 8, 2019
View reviewed changes
src/shared/lib/svgToPdfDownload.ts Outdated
req.end((err, res)=>{
if (!err && res.ok) {
fileDownload(base64ToArrayBuffer(res.text), filename);
// dealing with oncoprint svg (temporarily)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is temporary b/c @adamabeshouse will fix in actual library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct. when we have the new oncoprint.js, these lines can be removed from here.

alisman
alisman reviewed Mar 8, 2019
View reviewed changes
src/shared/lib/svgToPdfDownload.ts
}

export async function svgToPdfPromise(svg:Element, servletUrl?: string) {
const res = await svgToPdfRequest(svg, servletUrl);
export async function svgToPdfPromise(svg:Element) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we get rid of this?

Copy link
Member Author

@dippindots dippindots Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This promise is required by the charts in study view page, may not easy to change.

@dippindots dippindots marked this pull request as ready for review March 8, 2019 18:48
@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 18, 2019 20:57 Inactive
@tmazor
Copy link
Contributor

tmazor commented Mar 18, 2019

So far this looks good except when I downloaded the enrichments volcano plot as a PDF something went very wrong with the text....
image

@tmazor
Copy link
Contributor

tmazor commented Mar 18, 2019

A few issues on study view:

The mutation vs CNA plot doesn't have a PDF download option.

The "<=" sign doesn't come through correctly:
image

Superscript gets lost - these numbers are 10, 10^2, 10^3 and 10^4:
image

@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 21, 2019 16:35 Inactive
@dippindots
Copy link
Member Author

@tmazor Thank you so much for helping to find these issues. The issues have been solved by changing to a custom font(FreeSerif), this font can handle most of the special characters. Could you help me to review my pr again? thanks in advance! Here is the link: https://cbioportal-frontend-pr-2094.herokuapp.com/

@dippindots dippindots requested a review from tmazor March 21, 2019 18:08
@tmazor
Copy link
Contributor

tmazor commented Mar 21, 2019

Thanks @dippindots !

I don't really love the fact that we now have a mix of fonts in some of the PDF downloads (I'm having trouble with uploading images, but take a look at the PDF download of the plot on the co-expression tab and note how the title is a different font than everything else), and the predominant font in the PDF downloads is now a serif font when the website (and png/svg downloads) all use a sans serif font.

I realize this is a consequence of using FreeSerif to fix the problems with the special characters. Is there a sans serif option that we could use instead?

This isn't a deal breaker for me - so if freeSerif if the best option to get PDF downloads working again, I can live with this. But I'd be happier with more consistency in the fonts :)

@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 21, 2019 20:59 Inactive
tmazor
tmazor approved these changes Mar 21, 2019
View reviewed changes
Copy link
Contributor

@tmazor tmazor 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 to me @dippindots !

alisman
alisman reviewed Mar 22, 2019
View reviewed changes
src/pages/studyView/chartHeader/ChartHeader.tsx
@@ -78,8 +79,10 @@ export class ChartHeader extends React.Component<IChartHeaderProps, {}> {
const fileName = `${this.fileName}.${props.type.substring(0, 3).toLowerCase()}`;
if (props.type === "PNG") {
saveSvgAsPng(data, fileName, {backgroundColor:"#ffffff"});
} else if ((props.type === "PDF" || typeof data === "string") && data.length > 0) {
} else if (typeof data === "string" && data.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what type is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@alisman It is for SVG type, SVG will be serialized to string.

@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 22, 2019 19:27 Inactive
@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 22, 2019 20:53 Inactive
@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 22, 2019 20:55 Inactive
@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 25, 2019 15:47 Inactive
@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2094 March 25, 2019 16:00 Inactive
@alisman
Copy link
Collaborator

alisman commented Mar 25, 2019

There are three failing e2e tests that fail with timeout. They are not connected with this PR.

@alisman alisman merged commit 70b8c5e into cBioPortal:master Mar 25, 2019
@dippindots dippindots mentioned this pull request Mar 25, 2019
Closed
@dippindots dippindots deleted the replace_pdf_with_svg2pdf branch March 25, 2019 19:46
@jjgao jjgao mentioned this pull request Nov 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmazor tmazor tmazor approved these changes

@alisman alisman alisman approved these changes

Assignees

@dippindots dippindots

Labels
ready to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dippindots @tmazor @alisman @jjgao