Conversation
| def bootstrap_deps_suppress( | ||
| parts: list[Literal["css", "js", "meta"]] | ||
| ) -> list[HTMLDependency]: | ||
| bs_v_suppressed = str(bootstrap_version) + ".9999" | ||
|
|
||
| return [ | ||
| HTMLDependency(name=f"bootstrap-{part}", version=bs_v_suppressed) | ||
| for part in parts | ||
| ] |
There was a problem hiding this comment.
This is a small thing, but I think I'd prefer this to be more generic, like htmltools::supressDependencies()?
There was a problem hiding this comment.
Does that not exist in py-htmltools? We could add it there.
There might still be some value in this helper function, though. This function encapsulates both the Bootstrap version and the naming pattern we use. Also, suppressDependencies() uses version="9999", but I'm trying to be more careful here and am only suppressing the version used by Shiny. So we'd either have to update the API of suppressDependencies() or we'd still use this version.
OTOH, we could drop this function and other packages could use something like htmltools.suppress_dependency() instead. As we can see from the shinyswatch experience though, that increases the strength of coupling between shiny and the external packages. If we change how the Bootstrap dependency is structured, we'd need to change third party code as well. If we use this (not publicly advertised) helper function though, we can change the implementation in Shiny without breaking external code or having to update across several packages at once
There was a problem hiding this comment.
Ahhh, now I see that your intention is for {shinyswatch} to also make use of this function. I suppose that's good enough reason for keeping this as is so that shinyswatch doesn't have to know how the dependency names are setup.
Also, let's add a comment about shinyswatch using this?
| } | ||
|
|
||
| bootstrap_css = HTMLDependency( | ||
| name="bootstrap-css", |
There was a problem hiding this comment.
I would consider pulling the names out into variables so that it's more obvious that bootstrap_deps() and bootstrap_deps_suppress() have shared logic
|
|
||
| @add_example() | ||
| def include_bootstrap_css( | ||
| theme: Path | str | HTMLDependency, |
There was a problem hiding this comment.
Seems we could keep things a lot simpler if we dropped support for this type. I'm not entirely sure we can get away with that though
| theme: Path | str | HTMLDependency, | |
| theme: Path | str, |
| return [ | ||
| *bootstrap_deps_suppress(["css"]), | ||
| theme_dep, | ||
| ] |
There was a problem hiding this comment.
I now realize that this'll be problematic for multiple calls to include_bootstrap_css(). Maybe we could work around that by always returning the full Bootstrap dependency, but with the version incremented by f".counter()" each time?
| version=theme.version, | ||
| ) | ||
|
|
||
| if hasattr(theme, "bs_version"): |
There was a problem hiding this comment.
I think some of my concerns about complexity here could go away if we decided we didn't need to introduce yet another representation of the version
2 participants
Closes #1270