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

Set RNG seed for htmlwidgets IDs #2294

Merged
merged 2 commits into from
Oct 18, 2023
Merged

Set RNG seed for htmlwidgets IDs #2294

merged 2 commits into from
Oct 18, 2023

Conversation

salim-b
Copy link
Collaborator

@salim-b salim-b commented Apr 2, 2023

htmlwidgets::setWidgetIdSeed() needs to be explicitly called for stable htmlwidget IDs due to how htmlwidgets:::createWidgetId() works.

This reduces noise in pkgdown reference HTML output when examples generate htmlwidgets. Note that this change makes only the id attribute of the outer <div> container of an htmlwidget stable, and not its content, which could still remain irreproducible due to other reasons (this is e.g. the case for plotly charts).

The second commit in this PR contains cosmetic changes which are unrelated to PR's goal.

@hadley
Copy link
Member

hadley commented Oct 17, 2023

Could you please give me a PR without the other cosmetic changes? And with add a news bullet 😄

@salim-b salim-b mentioned this pull request Oct 17, 2023
`htmlwidgets::setWidgetIdSeed()` needs to be explicitly called for this due to how `htmlwidgets:::createWidgetId()` works
@salim-b
Copy link
Collaborator Author

salim-b commented Oct 17, 2023

@hadley I've updated the PR accordingly.

@@ -15,7 +15,7 @@ Test spacing above widget.

```{r, echo=FALSE}
# set seed for reproducible widget id
if (requireNamespace("htmltools", quietly = TRUE)) {
if (requireNamespace("htmlwidgets", quietly = TRUE)) {
htmlwidgets::setWidgetIdSeed(42)
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 drop this block now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope. The htmlwidgets seed init in R/build-reference.R above only applies to htmlwidgets that are generated in examples (built by build_reference()), not vignettes/articles (built by build_articles()). Currently, only the former function offers a seed param.

I think we need to modify the envir passed to rmarkdown::render() if we also want to have stable htmlwidget IDs for articles by default:

envir = globalenv(),

Should I give it a try? If yes, in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's do that in a separate PR. I'll merge this one momentarily. Thanks for working on this!

@hadley hadley merged commit 8699d86 into r-lib:main Oct 18, 2023
13 checks passed
@salim-b salim-b deleted the htmlwidget-seed branch October 18, 2023 16:57
SebKrantz pushed a commit to SebKrantz/pkgdown that referenced this pull request Jun 1, 2024
`htmlwidgets::setWidgetIdSeed()` needs to be explicitly called for this due to how `htmlwidgets:::createWidgetId()` works
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