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

Should theme argument in page_*() functions accept a string? #1067

Open
daattali opened this issue Jun 3, 2024 · 6 comments
Open

Should theme argument in page_*() functions accept a string? #1067

daattali opened this issue Jun 3, 2024 · 6 comments

Comments

@daattali
Copy link
Contributor

daattali commented Jun 3, 2024

In shiny's page functions, the theme argument can accept either a bs_theme() object or a path to a CSS file.

In bslib's page functions, the documentation says that the only acceptable value is a bs_theme() object. However, it seems that providing a string path to a CSS file also works. Should this be accepted or no? If yes, it should be documented. If no, an error should get thrown.

@cpsievert
Copy link
Collaborator

I think the ship has probably sailed at this point for accepting a string. We also do throw an informative error:

> bslib::page("bar", theme = "foo")
Error in assert_bs_theme(theme) : `theme` must be a `bs_theme()` object

@daattali
Copy link
Contributor Author

daattali commented Jun 3, 2024

Strange, I tested on both the CRAN version and the latest github version of {bslib}, and ui <- page(theme = "style.css", "test") does work for me. No error message, and it applies the stylesheet.

@cpsievert cpsievert reopened this Jun 3, 2024
@cpsievert
Copy link
Collaborator

Ahh, right, that error only gets thrown when rendered statically. Otherwise, it gets passed to shiny::bootstrapPage(), which doesn't error. Maybe we should do more to throw earlier on.

@gadenbuie
Copy link
Member

The other side of this would be to allow a string value for theme, in which case theme should point to a static CSS file, i.e. a pre-compiled Bootstrap theme. This would be in line with how the theme argument works in Shiny for Python page functions. But it would require more thinking (and also probably improved utilities for saving a bs_theme() to a set of static files, which of course brings additional issues with Shiny's built-in components that do use bs_dependency()).

Clearly throwing an error would be easier, but I think we should contemplate alternatives before implementing.

@daattali
Copy link
Contributor Author

What is meant by "static rendering"? What is the dynamic rendering version of the code I gave above ui <- page(theme = "style.css", "test")?

@gadenbuie
Copy link
Member

What is meant by "static rendering"?

I think Carson means printing ui or otherwise rendering it to html, where dynamic rendering would be using ui in a Shiny app.

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

No branches or pull requests

3 participants