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 htmlwidgets in Examples #343

Closed
wants to merge 8 commits into from
Closed

Conversation

saurfang
Copy link

I just want to thank you first for such an awesome package. It adds so much flesh to the documentation.

Currently, when HTML and htmlwidgets are called in RStudio, they automatically render in RStudio viewer and fails to be captured by evaluate.

This PR bypasses the print for these two classes and instead save them to a local HTML file. The HTML is then embedded as an iframe in the reference.

p.s. I want to gather your thoughts first while I work on the test cases.

@hadley
Copy link
Member

hadley commented May 18, 2017

Interesting idea! @jjallaire what do you think of this approach?

@@ -113,6 +113,29 @@ replay_html.recordedplot <- function(x, name_prefix, obj_id, ...) {
paste0("<img src='", escape_html(path), "' alt='' width='540' height='400' />")
Copy link
Author

@saurfang saurfang May 18, 2017

Choose a reason for hiding this comment

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

on an unrelated note, this fixed aspect ratio can make the plot look a bit funny, although I don't see it to be a problem on ggplot2 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Why? We're setting the graphics device size above.

Copy link
Author

Choose a reason for hiding this comment

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

You are totally right. And I cannot reproduce what I saw anymore 😨 maybe I misremembered.

@jjallaire
Copy link

This shouldn't be necessary (it's possible to fully capture all of the dependencies and include them in the pkgdown page tag). The entire model of htmlwidgets presumes multiple rendering environments and can definitely be adapted to pkgdown with a bit of extra effort.

@jjallaire
Copy link

Note that HTML and htmlwidgets do in fact get capture by knitr/evaluate. They end up hitting the knit_print generic which ends up populating the html dependencies knit meta, which is then scooped up by the rendering environment, be it rmarkdown::render, bookdown, RStudio Viewer, or whatever. What's happening in pkgdown is that this info is just dropped on the floor. If we preserve it and inject it into the <head> everything will work as expected.

@saurfang
Copy link
Author

Thanks for the pointers @jjallaire ! Before I try your approach, can you elaborate a little on the advantage? Is it just against using iframe or is there any other reasons?

It seems it will require propagating HTML dependency attributes all the way through reply_html/as_data/data_reference_topic functions and so wisker can fill a new placeholder in head.html. Are you proposing change evaluate::evaluate output handler to use knitr::knitr_print for all or still need the special check of the class that I am doing now and only do so for htmlwidgets/html for knitr_asis output type.

@jjallaire
Copy link

iframes are okay if a bit slower due to reloading an entire set of dependencies in each iframe) when htmlwidgets have a fixed size, but many widgets have varying heights (e.g. DT) and it's really, really hard to lay these out in iframes (there are libraries that allow two-way handshaking between iframe parents and iframe content to work out sizing but I don't know how well they work).

I'd argue that the easiest way to get html dependencies working is just to let rmarkdown::render handle the entire page. I realize though that pkgdown has a commitment to whisker and supporting other page composition environments like jekyll so that may not be practical.

Note that you can call rmarkdown::render with run_pandoc = FALSE (https://github.com/rstudio/rmarkdown/blob/master/R/render.R#L26) and you will get back the knit meta context that has all of the html dependencies (this is what both bookdown and the learnr package do). Then those dependencies can be fed though to whisker, etc.

@hadley
Copy link
Member

hadley commented May 19, 2017

@jjallaire so we'd generate an Rmd file including the examples, and then render that instead?

@jjallaire
Copy link

jjallaire commented May 19, 2017 via email

@hadley
Copy link
Member

hadley commented May 19, 2017

@saurfang do you want to have a go at this? You'd need to replace print() with knit_print(), return the extra metadata as an attribute, and then in replay_html() collect all the metadata and combine together. If not, I'll tackle at some point in the future.

@saurfang
Copy link
Author

Makes sense. Not right now. Maybe next week or so. Thanks for the help!

@saurfang
Copy link
Author

This almost works. https://saurfang.github.io/pkgdown.example/reference/index.html

Except that DT doesn't render style correctly
https://saurfang.github.io/pkgdown.example/reference/datatable.html
and plotly jams in an unreproducible random id that makes pkgdown keeps generating different HTML (when Rd changes and even if the example output doesn't change).
https://saurfang.github.io/pkgdown.example/reference/ggplotly.html
https://github.com/ropensci/plotly/blob/2907bd40a715e203c3a4f09aa9c63be38cb28274/R/plotly.R#L364
https://github.com/ropensci/plotly/blob/15cd04927b8ee53b562601644e7ac66521c2b965/R/utils.R#L90

I'll take further look at the second issue and polish my test package a little. Any chance you guys have insights to the first one? I banged my head for a little while on this already.

@jjallaire
Copy link

jjallaire commented May 26, 2017 via email

@hadley
Copy link
Member

hadley commented Jun 12, 2017

It looks like DT is having problems because it's embedded inside a pre.

Forest Fang added 6 commits June 12, 2017 22:04
Currently when HTML and htmlwidgets are called in RStudio, they automatically render in RStudio viewer and fail to be captured by `evaluate`.

This bypasses the `print` for these two classes and instead save them to a local html file. The html is then embedded as an iframe in the reference.
Freeze the outer id to obj_id to avoid crazy diffs.
This might be benefitial in case there are nested widgets in the future.
Use `knit_print` to print examples so that HTML output can be correctly captured. The dependencies are pryed off from `knit_meta`, collected and injected in `head.html` template.
This prevents `pre` tampering style for `knit_asis` output, which is often htlmwidgets and can have strict styling requirments.
@saurfang
Copy link
Author

You are right. I can see if I close the pre tag, it would render fine. However, I couldn't figure out which style rule is causing it. Would you consider this acceptable though (by closing and open pre just for knit_asis output)?
https://saurfang.github.io/pkgdown.example/reference/datatable.html

The naive implementation leaves an empty pre at the end and I can work to remove. Just want to get some feedback on this approach first.

R/rd-data.R Outdated
expr <- evaluate::evaluate(text, code_env, new_device = TRUE)
output_handler <- evaluate::new_output_handler(
value = function(x) {
if (isS4(x)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this branch really necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. It looks like the knit_print.default calls knitr:::normal_print() which already took care of this.

R/replay-html.r Outdated
pieces[i] <- replay_html(parts[[i]], obj_id = i, ...)
output <- replay_html(parts[[i]], obj_id = i, ...)
dependencies[[i]] <- attr(output, "knit_meta")
pieces[i] <- output
Copy link
Member

Choose a reason for hiding this comment

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

Should also use [[

Copy link
Author

Choose a reason for hiding this comment

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

I thought we generally use [ for primitive vectors?

@@ -113,6 +113,29 @@ replay_html.recordedplot <- function(x, name_prefix, obj_id, ...) {
paste0("<img src='", escape_html(path), "' alt='' width='540' height='400' />")
Copy link
Member

Choose a reason for hiding this comment

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

Why? We're setting the graphics device size above.

R/replay-html.r Outdated
@@ -113,6 +137,14 @@ replay_html.recordedplot <- function(x, name_prefix, obj_id, ...) {
paste0("<img src='", escape_html(path), "' alt='' width='540' height='400' />")
}

#' @export
replay_html.knit_asis <- function(x, name_prefix, obj_id, ...) {
output <- paste0("</pre><div class='well knit_asis'>", x, "</div><pre class=\"example\">")
Copy link
Member

Choose a reason for hiding this comment

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

We need to cross-reference this with the example starting and ending tags to make sure it stays consistent.

Copy link
Author

Choose a reason for hiding this comment

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

By "cross-referece", do you mean applying examples css class?

@@ -6,6 +6,9 @@

<!-- jquery -->
<script src="https://code.jquery.com/jquery-3.1.0.min.js" integrity="sha384-nrOSfDHtoPMzJHjVTdCopGqIqeYETSXhZDFyniQ8ZHcVy08QesyHcnOUpMpqnmWq" crossorigin="anonymous"></script>
<script type="text/javascript">
var jQuery_3_1_0 = $.noConflict(false);
Copy link
Member

Choose a reason for hiding this comment

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

Can we just make this jquery?

Copy link
Author

Choose a reason for hiding this comment

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

I think so because pkgdown.js comes before HTML dependencies so it wouldn't have been overridden yet. I also put it inside an anonymous function to scope it better.

1. Remove import from methods
2. Correctly apply css class and styles
3. Hide empty dangling pre tag
4. Relocate jQuery noConflict call
Copy link
Author

@saurfang saurfang left a comment

Choose a reason for hiding this comment

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

All comments addressed. Everything looks good from my end: https://saurfang.github.io/pkgdown.example/reference/datatable.html

Let me know if you have any other thoughts or concerns. Otherwise, I can squash before merging.

I also filed an issue for plotly's unreproducible object: plotly/plotly.R#1048

@@ -113,6 +113,29 @@ replay_html.recordedplot <- function(x, name_prefix, obj_id, ...) {
paste0("<img src='", escape_html(path), "' alt='' width='540' height='400' />")
Copy link
Author

Choose a reason for hiding this comment

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

You are totally right. And I cannot reproduce what I saw anymore 😨 maybe I misremembered.

R/rd-data.R Outdated
expr <- evaluate::evaluate(text, code_env, new_device = TRUE)
output_handler <- evaluate::new_output_handler(
value = function(x) {
if (isS4(x)) {
Copy link
Author

Choose a reason for hiding this comment

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

Good point. It looks like the knit_print.default calls knitr:::normal_print() which already took care of this.

R/replay-html.r Outdated
pieces[i] <- replay_html(parts[[i]], obj_id = i, ...)
output <- replay_html(parts[[i]], obj_id = i, ...)
dependencies[[i]] <- attr(output, "knit_meta")
pieces[i] <- output
Copy link
Author

Choose a reason for hiding this comment

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

I thought we generally use [ for primitive vectors?

R/replay-html.r Outdated
@@ -113,6 +137,14 @@ replay_html.recordedplot <- function(x, name_prefix, obj_id, ...) {
paste0("<img src='", escape_html(path), "' alt='' width='540' height='400' />")
}

#' @export
replay_html.knit_asis <- function(x, name_prefix, obj_id, ...) {
output <- paste0("</pre><div class='well knit_asis'>", x, "</div><pre class=\"example\">")
Copy link
Author

Choose a reason for hiding this comment

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

By "cross-referece", do you mean applying examples css class?

@@ -6,6 +6,9 @@

<!-- jquery -->
<script src="https://code.jquery.com/jquery-3.1.0.min.js" integrity="sha384-nrOSfDHtoPMzJHjVTdCopGqIqeYETSXhZDFyniQ8ZHcVy08QesyHcnOUpMpqnmWq" crossorigin="anonymous"></script>
<script type="text/javascript">
var jQuery_3_1_0 = $.noConflict(false);
Copy link
Author

Choose a reason for hiding this comment

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

I think so because pkgdown.js comes before HTML dependencies so it wouldn't have been overridden yet. I also put it inside an anonymous function to scope it better.

@saurfang saurfang changed the title [WIP] Replay HTML and htmlwidgets as iframe Embed htmlwidgets in Examples Jun 16, 2017
R/replay-html.r Outdated
#' @export
replay_html.knit_asis <- function(x, name_prefix, obj_id, ...) {
# close and open pre tags because it sometimes breaks stylesheet of htmlwidgets
output <- paste0("</pre><div class='examples'><div class='knit_asis'>", x, "</div></div><pre class='examples'>")
Copy link
Member

Choose a reason for hiding this comment

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

By cross-reference, I mean that we've now defined how to open and close and example block in two places. If we forget, and only update one, we'll get a weird display problem that probably won't get noticed for a while.

pieces[i] <- replay_html(parts[[i]], obj_id = i, ...)
output <- replay_html(parts[[i]], obj_id = i, ...)
dependencies[[i]] <- attr(output, "knit_meta")
pieces[[i]] <- output
Copy link
Member

Choose a reason for hiding this comment

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

The rule is to use [[ whenever you're extracting a single value.

Copy link
Author

@saurfang saurfang left a comment

Choose a reason for hiding this comment

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

Understood. Instead of pasting everything together and wrapping pre in whisker, I pulled the pre wrapping inside so we avoid the open/close hack for HTML pieces.

) %>%
purrr::flatten() %>%
purrr::map_chr(as.character) %>%
unname()
Copy link
Author

Choose a reason for hiding this comment

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

I tried the functional approach, which doesn't look great IMO. If I implement it imperatively by looping through elements and merging, the logic is probably even less readable. I can also implement this as reduce_right by constantly checking the class of the first element in the accumulator. Let me know if you have any suggestions.

<pre class="examples">{{{ . }}}</pre>
{{#bundles}}
<div class="examples">{{{ . }}}</div>
{{/bundles}}
Copy link
Author

@saurfang saurfang Jun 16, 2017

Choose a reason for hiding this comment

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

Is there a way in mustache to avoid this nesting?

Right now I had to make it like so so that the anchor for Examples only shows if the examples are present:

{
  "examples": {
    "bundles": [
       "...bundle 1...",
       "...bundle 2...",
       "..."
    ]
  }
}

@KasperSkytte
Copy link

Hi!

First, thanks for an awesome package! It takes documentation to the next level!
I'm not really into the details you discuss here, but I seem to have a similar issue with ggplotly objects in the examples section of a function not showing up, it gets rendered in the RStudio viewer instead. It works fine if embedded in a markdown page in Articles, however, but then the current section indicated at the sidebar menu doesn't highlight when scrolling down the page. I can provide more information about what I've done if needed, but I believe it should work as-is when simply running ggplotly(any-ggplot2-object) within the examples section of a function?

I'm running Ubuntu 16.04 LTS with everything updated to the newest version as of today.

@saurfang
Copy link
Author

This was in a working state minus the parts I mentioned before that were beyond pkgdown's control. I can rebase this if Hadley has the bandwidth to review and merge this.

In your case, with this PR plotly widgets will render correctly but the source will change during every render due to unreproducible random seed used to generate widget id.

@hadley hadley mentioned this pull request Apr 2, 2018
@hadley
Copy link
Member

hadley commented Apr 24, 2018

I'm going to close this since pkgdown implementation has now changed radically since this was proposed. Sorry for not finding the time to merge it earlier!

@hadley hadley closed this Apr 24, 2018
jayhesselberth added a commit that referenced this pull request Jun 1, 2018
Based on previous work in #343 by @saurfang

Closes #617
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.

None yet

4 participants