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

Embed gifs in vignettes #1742

Closed
wants to merge 4 commits into from
Closed

Embed gifs in vignettes #1742

wants to merge 4 commits into from

Conversation

IndrajeetPatil
Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil commented Oct 20, 2022

No need to use still images.

If you knit this locally, you can see that the gifs work just fine.

If you want to see an example of a package on CRAN that includes gif in this way, see this.

No need to use still images.

If you knit this locally, you can see that the gifs work just fine.
@codecov-commenter

This comment was marked as off-topic.

@IndrajeetPatil
Copy link
Collaborator Author

On R 3.5 and R 3.4, this fails because R CMD check produces a NOTE:

* checking installed package size ... NOTE
  installed size is  5.4Mb
  sub-directories of 1Mb or more:
    R     1.4Mb
    doc   3.4Mb

Not sure how to proceed here. CRAN doesn't even check on these R versions, so not sure if it makes sense to pay heed to this NOTE.

Comment on lines +77 to +78
# with:
# error-on: '"note"'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😬

    happy to test this out, we may revert it slows us down a lot

Originally posted by @MichaelChirico in #1621 (review)

@AshesITR
Copy link
Collaborator

I remember we did exactly that (static images) to reduce the size at basically no loss of information.

@AshesITR
Copy link
Collaborator

#1325

@IndrajeetPatil
Copy link
Collaborator Author

at basically no loss of information

Not sure what you mean. Vignettes don't have the gifs on CRAN, so there is a loss of information, no?

With the current approach, it's a win-win. Both for CRAN vignette and pkgdown website.

@AshesITR
Copy link
Collaborator

Yes, but the gif's don't contain any important information, so I'd rather not violate installed size guidelines because of them. If you manage to get them to < 1MB in total, it'd be cool though 😊

@IndrajeetPatil
Copy link
Collaborator Author

Hmm, in that case, should we just get rid of the gifs and only include stills?

If there is no loss of information, what's the added value of including the gifs?

@AshesITR
Copy link
Collaborator

It's prettier on lintr.r-lib.org where there is no file size restriction to 1MB. That's what the current code does, conditionally show the animated versions online.

@IndrajeetPatil
Copy link
Collaborator Author

Yes, I get that, but it comes at the cost of an additional soft dependency (note that this is why "noSuggests" check is currently failing) and added complexity of maintaining different versions of the docs.

@IndrajeetPatil
Copy link
Collaborator Author

We don't have gifs for RStudio, VS Code, and Atom, either; just the screenshots. So we might as well be consistent and do the same for other IDEs.

@AshesITR
Copy link
Collaborator

Error: Failed to download Pandoc 2.14.2: Error: connect ETIMEDOUT 185.199.109.133:443

What's the problem root cause of the failure?

@IndrajeetPatil
Copy link
Collaborator Author

Error: Failed to download Pandoc 2.14.2: Error: connect ETIMEDOUT 185.199.109.133:443

@AshesITR Where are you seeing these errors? Based on the error message, looks like a transitory issue?

@AshesITR
Copy link
Collaborator

In the PR you linked that's the only build failure.

@IndrajeetPatil
Copy link
Collaborator Author

Oh, that was a spurious build failure; I've retriggered it. The actual issue you can see in the first commit build as well:

Error: --- re-buildingcontinuous-integration.Rmdusing rmarkdown
--- finished re-buildingcontinuous-integration.Rmd--- re-buildingcreating_linters.Rmdusing rmarkdown
--- finished re-buildingcreating_linters.Rmd--- re-buildingeditors.Rmdusing rmarkdown
Quitting from lines 26-32 (editors.Rmd) 

@AshesITR
Copy link
Collaborator

Seems like we should copy pkgdown::in_pkgdown() then?

@IndrajeetPatil
Copy link
Collaborator Author

I still think it'd be better to consistently have gifts or stills for all editors, not gifs for some, while still for others. But punting that for now.

Closing in favour of #1747.

@IndrajeetPatil IndrajeetPatil deleted the gifs_in_vignettes branch November 3, 2022 20:05
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.

3 participants