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

prefer webshot2 to webshot and solve #1858 #1918

Merged
merged 6 commits into from
Jan 29, 2021
Merged

Conversation

atusy
Copy link
Collaborator

@atusy atusy commented Nov 11, 2020

With this PR, html_screenshot` tries to use webshot2 and fallbacks to webshot.

Also, user may prioritize webshot by opts_chunk$set(webshot = "webshot").
Another possibility is to use opts_knit$set(webshot = "webshot").
I'm fine with both ways, but choose the opts_chunk because it has controlls on each chunks.

Copy link
Owner

@yihui yihui 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. Just one small question. Thank you!

R/plot.R Outdated Show resolved Hide resolved
@cderv cderv linked an issue Jan 29, 2021 that may be closed by this pull request
Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Great. I'll carry on from here and merge in a few minutes. Thank you!

@yihui yihui merged commit 20f8571 into yihui:master Jan 29, 2021
yihui added a commit that referenced this pull request Jan 29, 2021
@yihui
Copy link
Owner

yihui commented Jan 29, 2021

I forgot to mention that another possible idea is to use pagedown::chrome_print(). We have exported pagedown::find_chrome() there, and it is a CRAN package. I guess the only problem is the compatibility of arguments.

@cderv
Copy link
Collaborator

cderv commented Jan 29, 2021

The function are a bit different in their way to find chrome, and specifically with the env var mechanism which is not the same env var. They both will find chrome but there could be a mismatch of version if you have several locally, as webshot2 will use the one that chromote will use.
Also webshot:::find_phantom() was unexported - does feel bad to do the same.

I would stick to the one in chromote for now, and maybe ask them to put one in webshot2 as a helper to test availability of chrome for other package ? or better that they do the check themselves maybe.

@cderv
Copy link
Collaborator

cderv commented Feb 18, 2021

find_chrome is now exported from chromote. Not yet from webshot2 I believe.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Support for RStudio/Webshot2 instead of Webshot?
3 participants