-
Notifications
You must be signed in to change notification settings - Fork 117
feat: Add theme argument to page functions
#1334
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
Conversation
Expands the carve-outs to include `Tagifiable`, `HTMLDependency`, and `list[HTMLDependency]`. This improves usability for shinyswatch, but also hides the `ThemeProvider` type behind a type alias, pushing focus on the typical `str | Path` use case.
| "css", | ||
| ] | ||
|
|
||
| subprocess.run(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If purgecss isn't installed, you'll see: FileNotFoundError: [Errno 2] No such file or directory: 'purgecss'.
Probably worth a check + more informative error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't officially part of the example, just used to create it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note at the top of the file that purgecss is required and how to install it
cpsievert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! Let's add an to the changelog before merging?
Fixes #1270
This approach supercedes #1275 as a smaller, more targeted change. This PR:
themeargument toshiny.ui.page_()functions.themeis provided, i.e. is notNone, it represents a complete replacement for Bootstrap's CSS files.shinyswatch, theme accepts a tagifiable, or html dependency or a list of html dependencies.The scope of this PR is different from #1275 in that
themeis exclusively for the replacement of Bootstrap's CSS, all other scenarios are easily accomplished usingui.include_css()or html dependencies.If moving forward with this iteration, we also aren't going to separate the Bootstrap dependency into
bootstrap-cssandbootstrap-js. That approach is inconsistent with how we handle dependencies otherwise and possibly points to a need for anHTMLDependencyPartial.Closes #1275 (supercedes)
Closes #1282 (supercedes)