pdf options magicformat, emulatemedia, scale and injectjs #13
Conversation
wingsuitist
commented
Jan 31, 2020
wingsuitist
commented
- magicformat creates a pdf page format adapted to the website size
- emulatemedia and scale expose the matching puppeteer options
- injectjs injects a javascript from a file path before generating the pdf
| if (argv.magicformat) { | ||
| height = (await page.evaluate( | ||
| 'Math.max(document.body.scrollHeight, document.body.offsetHeight, document.documentElement.clientHeight, document.documentElement.scrollHeight, document.documentElement.offsetHeight)')); | ||
| width = '1366'; |
There was a problem hiding this comment.
how was this value chosen? should we make it an option?
There was a problem hiding this comment.
Based on the most common browser resolution website designs focus on.
As there is no width option, we'd have to add a separate with.
index.js
Outdated
| string: true, | ||
| default: '' | ||
| }, | ||
| 'magicformat': { |
There was a problem hiding this comment.
Since this option is exclusive with format, could we make the syntax for this be --format=auto so that it's clear this replaces any value for format?
There was a problem hiding this comment.
Ok, adapted it. How can we add a comment/info that there is a --format=auto?
index.js
Outdated
| string: true, | ||
| default: '' | ||
| }, | ||
| 'injectjs': { |
There was a problem hiding this comment.
rename to inject-js to be consistent with other multi-term options (will materialize as argv.injectJs then)
index.js
Outdated
| @@ -31,6 +32,22 @@ const argv = require('yargs') | |||
| desc: 'Print an HTML file or URL to PDF', | |||
| builder: { | |||
| ...commonOptions, | |||
| 'emulatemedia': { | |||
There was a problem hiding this comment.
rename to emulate-media to be consistent with other multi-term options (will materialize as argv.emulateMedia then)
| @@ -31,6 +32,22 @@ const argv = require('yargs') | |||
| desc: 'Print an HTML file or URL to PDF', | |||
| builder: { | |||
| ...commonOptions, | |||
| 'emulatemedia': { | |||
| string: true, | |||
There was a problem hiding this comment.
configure this as a boolean
There was a problem hiding this comment.
It's not a boolean, it's "screen" "print" etc.
index.js
Outdated
| height = (await page.evaluate( | ||
| 'Math.max(document.body.scrollHeight, document.body.offsetHeight, document.documentElement.clientHeight, document.documentElement.scrollHeight, document.documentElement.offsetHeight)')); | ||
| width = '1366'; | ||
| argv.format = undefined; |
There was a problem hiding this comment.
let's treat argv as immutable, either vary what gets passed to page.pdf(...) with ternary operators or pull out a mutable variable to be built up procedurally like you did with width and height
index.js
Outdated
| await page.evaluate(`(async () => {${fs.readFileSync(argv.injectjs)}})()`); | ||
| } | ||
|
|
||
| if (argv.emulatemedia) await page.emulateMedia(argv.emulatemedia); |
There was a problem hiding this comment.
linter conflict here, please break out the inlining
| if (argv.emulatemedia) await page.emulateMedia(argv.emulatemedia); | |
| if (argv.emulatemedia) { | |
| await page.emulateMedia(argv.emulatemedia); | |
| } |
| const buffer = await page.pdf({ | ||
| path: argv.output || null, | ||
| format: argv.format, | ||
| width, height, |
There was a problem hiding this comment.
should we default all these to null here like with path to ensure that omitted options on our end don't prevent page.pdf() from applying its own default behavior?
There was a problem hiding this comment.
I'm not sure. They should be undefined if not set.
|
@wingsuitist these are awesome additions, thanks for contributing! Forgive my slow follow-up, would you mine making some of the tweaks I requested? |
|
Thx! I'll look at it asap. |
|
Here are the fixes to my best understanding. Thank you for the outstanding feedback. |
