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

fix: fix overriding device scale factor #40

Conversation

hatemhosny
Copy link
Contributor

Setting viewport was overriding puppeteer launchOptions that set device scale factor
https://github.com/tungs/timesnap/blob/master/index.js#L161

this does not allow improving image quality by setting --launch-arguments="--device-scale-factor=2", or alternatively --launch-arguments="--force-device-scale-factor=2", as mentioned in #30

This fixes it by extracting these options and adding them to config.viewport

You may consider also doing that for the rest of the properties
https://pptr.dev/#?product=Puppeteer&version=v5.0.0&show=api-pagesetviewportviewport

I believe this is an important setting for image quality, so you may also consider making it a cli arg

Setting viewport was overriding
launchOptions that set device scale factor
This fixes it by extracting these options
and adding them to config.viewport
@tungs tungs changed the base branch from master to device-scale-factor-viewport-fix January 12, 2021 18:59
@tungs tungs merged commit e8e8695 into tungs:device-scale-factor-viewport-fix Jan 14, 2021
@tungs
Copy link
Owner

tungs commented Jan 25, 2021

This change was integrated into v0.2.0, as well as allowing for additional viewport options to be defined in the CLI via the key=value, format e.g. --viewport="800,600,deviceScaleFactor=2". From my limited tests, it seems --launch-arguments="--device-scale-factor=2" or --launch-arguments="--force-device-scale-factor=2" does not seem to do anything more than change the deviceScaleFactor of the viewport, so it's probably a better practice to set the scale factor via the viewport option.

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.

2 participants