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

Add ability to export html as well (for SPA server-side rendering) #145

Merged
merged 2 commits into from
Dec 14, 2016

Conversation

adstage-david
Copy link
Contributor

@adstage-david adstage-david commented Dec 14, 2016

We're looking to replace our current setup for PDF generation with electron-pdf in our React-based single page app. Currently we use a two-step process to capture PDFs of our dashboards:

  1. export html via PhantomJS worker (this helps wkhtmltopdf, because some of what we've been doing with React and D3.js doesn't seem to render well directly in the old forked version of QTWebkit it relies on)
  2. convert the exported html to PDF

This process enabled us to also do server-side rendering for our dashboards, which is a neat way to allow HTML export for our React app in a way that doesn't require any JS for the end user. Ideally I'd like to tear out and replace the HTML export from PhantomJS and use the same Electron-based system for consistency, and this seemed like the easiest place to do it.

With these changes I can use the same event system to wait for both PDF and HTML to be rendered, etc.

Copy link
Owner

@fraserxu fraserxu left a comment

Choose a reason for hiding this comment

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

Nice work! Can you fix the syntax error that standard is complaining about?

fs.writeFile(target, result, function (err) {
this.emit('window.capture.end', {file: target, error: err})
this.emit('export-complete', {file: target})
// REMOVE pdf-complete in 2.0 - keeping for backwards compatibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should remove the comment and the 'pdf-complete' event now since no one could have relied on it for html exports. I named it poorly to begin with and was only keeping it for backwards compatibility.

@codecounselor
Copy link
Collaborator

Thanks for the PR, it might be good to mention this in the README.md as well. Would you mind adding something?

@codecounselor codecounselor added this to the 1.1.1 milestone Dec 14, 2016
@adstage-david
Copy link
Contributor Author

Feedback hopefully all addressed - thanks for looking at it so quickly!

@fraserxu
Copy link
Owner

👍 Looks good to me. I'm not using any of these features personally so I'll leave it to @codecounselor to merge.

@codecounselor codecounselor merged commit 44deb4c into fraserxu:master Dec 14, 2016
@codecounselor
Copy link
Collaborator

I have a couple of changes I am working on now, but will hopefully cut the 1.1.1 release within the week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants