-
Notifications
You must be signed in to change notification settings - Fork 23
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
Improve animint2pages by automatically taking screenshot of the animint #131
Conversation
Added line Chromote session initialization as a starting step for using chromote screenshot API
great thanks for sharing. can yoiu please convert the PR to draft until it is ready for review? |
added chromote in suggests for testing chromote's screenshot API
added chromote package for testing chromote screenshot API
changed the location of chromote init line in the code and added print to see local address of animint
used chromote's API of taking screenshot by navigating to url of local temporary directory
corrected variable name from chrome to chrome.session
changed chrome sessions captureScreenshot() API to chromote's screenshot() API to compare which option will be better.
added new test to see if screenshot.png file exists in github pages repo.
corrected hardcoded repo owner name to actual repo owner during the test by getting the repo owner from animint2pages
corrected hardcoded repo name to actual repo name during the test
test if gh-pages branch exist duting github actions testing
added code to write data from chromote screenshot api to local dir which has animint
code for server pointing to temp directory where dynamic html file is there
17b6ef9
to
1ec415b
Compare
I manually tested the functionality by running the code below, library(animint2)
mtcars$Cyl <- factor(mtcars$cyl)
viz <- animint(
ggplot(mtcars, aes(x = mpg, y = disp, color=Cyl)) +
geom_point(),
ggplot(mtcars, aes(x = hp, y = wt, color=Cyl)) +
geom_point(),
title="Motor Trend Cars data viz",
source="https://github.com/animint/animint2/blob/master/R/z_pages.R"
)
animint2pages(viz, "2024-08-29-screenshot-test") which gave me this viz https://tdhock.github.io/2024-08-29-screenshot-test/ and the screenshot below, That results in the image below, |
yes. now the screenshots are better. widgets are not necessary in the screenshot. thank you for pointing that out and making a commit. I will look into it to understand the changes you made |
R/z_pages.R
Outdated
res <- animint2dir(plot.list, open.browser = FALSE, ...) | ||
portNum <- servr::random_port() | ||
normDir <- normalizePath(res$out.dir, winslash = "/", mustWork = TRUE) | ||
code = sprintf("servr::httd(dir='%s', port=%d)", normDir, portNum) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this functionality (starting a server using servr::httd) seems duplicated here, and when printing animint, and in tests_init. is there some way to combine the repeated logic into a helper function?
…/animint2 into screenshot-animint2pages
@tdhock, I resolved all the comments. Can you please have another look to see if the PR is now ready to be merged ? |
hello @tdhock , is there anything more that needs to refactored before merging this PR ? |
NAMESPACE
Outdated
@@ -477,6 +478,7 @@ export(stat_summary_bin) | |||
export(stat_summary_hex) | |||
export(stat_unique) | |||
export(stat_ydensity) | |||
export(stop_server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two different spellings of server, in start_servr
and stop_server
, is potentially confusing. please change to stop_servr
for consistency.
great, thanks for your patience! this is a really useful contribution. |
This pull request is a part of GSoC 2024 task of adding the feature of automatic screenshot in animint2pages function.