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

Deprecate bootstrap_dependencies() and use bslib as default instead #2154

Open
apreshill opened this issue Jun 3, 2021 · 7 comments
Open
Labels
next to consider for next release theme: bootstrap related to bootstrap

Comments

@apreshill
Copy link
Contributor

(to discuss)

@cpsievert After discussion, we are considering adding bslib as a dependency for rmarkdown. Looking at the dependencies, there is a lot of overlap, but we were wondering about the magrittr dependency: https://github.com/rstudio/bslib/blob/master/DESCRIPTION#L28

We didn't see anything clear in the bslib docs on using magrittr with bslib- might that be moved to suggests instead? Or are there other use cases? Thanks!

@cpsievert
Copy link
Contributor

Yea, I can likely move magrittr to a suggestion in the next release

@cderv
Copy link
Collaborator

cderv commented Jun 3, 2021

I was curious about the relation about rmarkdown and magrittr so I looked it up.
Let's just know that magrittr is already a package that will be installed with rmarkdown thought the use of stringr. https://github.com/tidyverse/stringr/blob/f030ae09b96e6815e16e257bad69c735530a2479/DESCRIPTION#L26

> pak::pkg_deps_explain("rmarkdown", "magrittr")
rmarkdown -> knitr -> stringr -> magrittr                                 
rmarkdown -> stringr -> magrittr

But it seems indeed that bslib does not really need magrittr has a hard dependency so it would be best to move it to soft dep if possible.

@yihui
Copy link
Member

yihui commented Jun 3, 2021

IMHO infrastructure packages like bslib and stringr shouldn't be so opinionated about users' coding style (to pipe or not). This decision should be left to users (i.e., if they like pipes, they can always load magrittr by themselves), rather than being enforced at the package level (i.e., making magrittr a hard dependency).

FWIW the dependency on stringr might be removed someday (yihui/knitr#1549).

@cderv
Copy link
Collaborator

cderv commented Jun 3, 2021

Same as in bslib, I don't think this is used directly in the package. I believe this reexported by convenience in the tidyverse to have the operator loaded in the session when stringr is loaded without to have to load magrittr explicitly. But this is a low level package with no dependency, so it do no harm to have it available with those package without needing to add library(magrittr) everytime you need the pipe.

But I see and understand your argument for package outside the tidyverse ecosystem (which itself use the pipe exetensively by design I think)

@yihui
Copy link
Member

yihui commented Jun 3, 2021

On one hand, I have no problem with higher-level packages being opinionated. On the other hand, bslib doesn't belong to the tidyverse, hence it doesn't have to be as opinionated as tidyverse in terms of users' coding style. In addition, R has got the new native pipe |> now, so it makes even less sense for bslib to play the role of deciding for users which pipe to use.

@apreshill
Copy link
Contributor Author

Adding [TODOs] here:

  1. Deprecation strategy then needed for the existing exported Bootstrap dependency (https://pkgs.rstudio.com/rmarkdown/reference/html-dependencies.html)

    • Deprecate with warning and a message

    • Check for CRAN rev deps

  2. Decision needed: which version of Bootstrap should be the default- 3 or 4?

@cderv
Copy link
Collaborator

cderv commented Sep 8, 2021

FWIW bslib version 0.3.0 is out with dependencies removed.

Deps tree of bslib is now:

> pak::pkg_deps_tree("bslib")
bslib 0.3.0 [new]
+-htmltools 0.5.2 [new]
| +-digest 0.6.27 [new][dl] (268.58 kB)
| +-base64enc 0.1-3 [new]
| +-rlang 0.4.11 [new][dl] (1.19 MB)
| \-fastmap 1.1.0 [new][dl] (215.42 kB)
+-jsonlite 1.7.2 [new][dl] (544.22 kB)
+-sass 0.4.0 [new][dl] (3.64 MB)
| +-fs 1.5.0 [new][dl] (605.11 kB)
| +-rlang
| +-htmltools
| +-R6 2.5.1 [new][dl] (84.27 kB)
| \-rappdirs 0.3.3 [new][dl] (58.75 kB)
+-jquerylib 0.1.4 [new][dl] (525.79 kB)
| \-htmltools
\-rlang

Decision needed: which version of Bootstrap should be the default- 3 or 4?

Bootstrap 5 will be the default in next bslibrelease probably. So choice will be default to 3 or 5 maybe...

@yihui yihui added the next to consider for next release label Sep 8, 2021
yihui added a commit that referenced this issue Sep 14, 2021
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this issue Sep 17, 2021
Merge remote-tracking branch 'rstudio_origin/main' into jg-tree-fix

# By Yihui Xie (7) and others
# Via Yihui Xie
* rstudio_origin/main:
  import bslib (rstudio#2154)
  CRAN release v2.11
  https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html -> https://pandoc.org/MANUAL.html#citations
  Add a specific dirname for sass caching
  roxygenize and bump version
  export convert_ipynb() per suggestion of @acircleda
  use sass::output_template() instead of storing a copy in rmarkdown
  fix yihui/xaringan#331: respect relative paths in parent directories in the `css` argument of `html_document()`
  Bump version
  Prerender shiny rmd in separate environment (rstudio#2203)
  Mark result of citeproc conversion as UTF-8 (rstudio#2202)

# Conflicts:
#	NEWS.md
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this issue Sep 17, 2021
Merge branch 'jg-tree-fix' into jg-devel

# By Yihui Xie (9) and others
# Via Jonathan Gilligan (3) and Yihui Xie (1)
* jg-tree-fix:
  import bslib (rstudio#2154)
  CRAN release v2.11
  https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html -> https://pandoc.org/MANUAL.html#citations
  Add a specific dirname for sass caching
  roxygenize and bump version
  export convert_ipynb() per suggestion of @acircleda
  use sass::output_template() instead of storing a copy in rmarkdown
  fix yihui/xaringan#331: respect relative paths in parent directories in the `css` argument of `html_document()`
  Bump version
  Prerender shiny rmd in separate environment (rstudio#2203)
  Mark result of citeproc conversion as UTF-8 (rstudio#2202)
  Fixed conflicts with rstudio main version.
  Update documentation for html_document.
  Add self (JG) as a contributor in DESCRIPTION.
  Resolve conflicts with new updates to RStudio main branch.
  pin the jquery version to 3: rstudio#2197 (comment)

# Conflicts:
#	DESCRIPTION
#	NEWS.md
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this issue Sep 27, 2021
* rstudio_origin/main:
  Add support for devtools loaded package in `draft()` (rstudio#2224)
  Fix typo in comment
  import bslib (rstudio#2154)
  CRAN release v2.11
  https://rmarkdown.rstudio.com/authoring_bibliographies_and_citations.html -> https://pandoc.org/MANUAL.html#citations
  Add a specific dirname for sass caching
  roxygenize and bump version
  export convert_ipynb() per suggestion of @acircleda
  use sass::output_template() instead of storing a copy in rmarkdown
  fix yihui/xaringan#331: respect relative paths in parent directories in the `css` argument of `html_document()`
  Bump version
  Prerender shiny rmd in separate environment (rstudio#2203)
  Mark result of citeproc conversion as UTF-8 (rstudio#2202)
@cderv cderv changed the title [FR] Consider adding bslib an explicit default dependency Deprecate bootstrap_dependencies() and use bslib as default instead Dec 23, 2021
@cderv cderv added the theme: bootstrap related to bootstrap label Dec 23, 2021
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue May 5, 2022
# bslib 0.3.1

## New features

* Upgraded Bootstrap 5 (i.e., `bs_theme(version = 5)`) from 5.0.2 to
  5.1.0 (#365)

## Bug fixes

* Closed rstudio/shiny#3519: `nav_menu()` (i.e.,
  `shiny::navbarMenu()`) wasn't producing an `.active` class on it's
  `.dropdown` container properly. (#372)

# bslib 0.3.0

## Breaking changes

* Closed rstudio/rmarkdown#2154: `{magrittr}`'s pipe operator (`%>%`)
  is no longer re-exported by `{bslib}`. Either `library(magrittr)` to
  make `%>%` available and/or use use R 4.1's pipe operator (`|>`).

## New features

* Closed #82: Added support for Bootstrap 5 (via `bs_theme(version =
  5)`). Bootstrap 4 remains the default in this release, but the next
  release, the default will likely change to Bootstrap 5.

## Bug fixes

* Closed #6: rmarkdown's .tabset-fade class now works with Bootstrap
  4+ since legacy use of .nav .fade is now officially supported in
  Bootstrap 4+. (#325)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next to consider for next release theme: bootstrap related to bootstrap
Projects
Status: Backlog
Development

No branches or pull requests

4 participants