-
Notifications
You must be signed in to change notification settings - Fork 117
feat: Add ui.theme() and theme argument to page_*() functions
#1275
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
| theme_tag = head(include_css(theme)) | ||
| elif isinstance(theme, HTMLDependency): | ||
| theme_tag = theme | ||
| else: |
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 think we should check elif isinstance(theme, Tagifiable):?
| theme = Theme(theme) | ||
|
|
||
| if isinstance(theme, Theme): | ||
| theme.check_compatibility(bootstrap_version) |
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.
Could we do this check on __init__? Could be confusing that a "bad" them can be constructed, but then later throws an error (and probably leads to a confusing stack trace)
| + "When tagified, `theme` must return a `Tag` or `TagList`." | ||
| ) | ||
|
|
||
| self.theme: Tag | TagList | HTMLDependency = theme_tag |
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.
Probably worth commenting that Tag | TagList are here to support the ui.include_css() case
| from .._versions import bootstrap as BOOTSTRAP_VERSION | ||
| from ._include_helpers import include_css | ||
|
|
||
| __all__ = ("theme",) |
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 feel uneasy about having an exported shiny.ui.theme() that what is mainly a advanced user / developer facing API.
Also, I can't help but feel like Theme/theme naming is a bit of a misnomer, especially if we're thinking about this as a user facing API. In that case, maybe something like bootstrap_css() is better?
In fact, I hate to suggest this, but maybe we don't need a theme argument at all for this use case. Could we not have a bootstrap_css() that could be dropped anywhere in the UI? Implemented roughly like this?
__all__ = (
"bootstrap_css",
)
def bootstrap_css(path: Path | str, check_version: bool = True) -> list[Tag | HTMLDependency]:
# Snoop the version of the Bootstrap CSS file
if check_version:
check_bootstrap_version(path)
return [
ui.include_css(path),
HTMLDependency(
name="bootstrap-css", version=bootstrap_version
),
]
def check_bootstrap_version(path: Path | str):
file_path = check_path(path)
content = read_utf8(file_path)
# TODO: extract version from Bootstrap's banner, then warn/err if not compatible, and
# also warn if the version wasn't found?
def bootstrap_deps() -> list[HTMLDependency]:
return [
jquery_deps(),
HTMLDependency(
name="bootstrap-js",
version=bootstrap_version,
source={"package": "shiny", "subdir": "www/shared/bootstrap/"},
script={"src": "bootstrap.bundle.min.js"},
meta={"name": "viewport", "content": "width=device-width, initial-scale=1"},
),
HTMLDependency(
name="bootstrap-css",
version=bootstrap_version,
source={"package": "shiny", "subdir": "www/shared/bootstrap/"},
stylesheet={"href": "bootstrap.min.css"}
)
]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.
Would you mind moving this comment to the design issue?
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.
Two things to note about this approach:
-
We'd (attempt to) check the bootstrap version automatically. As long as Bootstrap isn't getting compiled in a weird way, this should work.
-
We'd (intentionally) drop the
replace = "all"feature, but we could bring it back with abootstrap_js()if need be down the road. In general though, the thought of encouraging users to customize the JS gives me the heebie jeebies.
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.
Oh sorry, just saw your comment, yes will do.
Closes #1270