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

refactor: Avoid needing to directly inspect shiny's bootstrap_deps() #32

Merged
merged 31 commits into from
Apr 10, 2024

Conversation

gadenbuie
Copy link
Contributor

@gadenbuie gadenbuie commented Apr 3, 2024

This PR is pre-work for posit-dev/py-shiny#1282 to avoid errors in Shiny's test suite caused by shinyswatch's assumptions about the now changing structure of bootstrap_deps().

The primary goal is to avoid having to inspect the Bootstrap dependencies provided by Shiny. Instead, we suppress the bootstrap, bootstrap-js and bootstrap-css dependencies in shinyswatch's html dependency.

In posit-dev/py-shiny#1282, Shiny's Bootstrap dependency will switch to bootstrap-js, bootstrap-css and bootstrap-meta, which can be suppressed with a new helper function shiny.ui._html_deps_external.bootstrap_deps_suppress().

The goal of posit-dev/py-shiny#1282 is to introduce shiny.ui.include_bootstrap_css() that could be used by shinyswatch in the future, although it's not strictly required. The approach used by include_bootstrap_css() is quite similar to the one taken here, except that its stated goal is to replace only the CSS and not the entire Bootstrap bundle. That function will also provide additional runtime checks to make sure that Shiny's Bootstrap is compatible.

In a future PR, I'll come back to shinyswatch and explore using the new function. In the mean time, the approach in this PR should work for both current and the expected future implementation in Shiny. As long as this PR doesn't break shinyswatch, I'd like to merge this so that work in Shiny can be tested.

New naming of shinyswatch dependency

The core shinyswatch deps are now named shinyswatch-js and shinyswatch-css. This way, if multiple themes are added, only one bootstrap.bundle.min.js and one bootswatch.min.css are ever loaded.

Re-worked theme picker

The theme picker now uses a separate HTMLDependency to load all of the themes at once. New JavaScript switches themes by replacing the initial theme with a new <link>. We actually add the new CSS and then remove the previous CSS with a slight delay, resulting in a smooth transition between themes.

Kapture.2024-04-05.at.09.30.38.mp4

@gadenbuie gadenbuie marked this pull request as draft April 4, 2024 19:47
@gadenbuie gadenbuie removed the request for review from schloerke April 4, 2024 19:47
@gadenbuie

This comment has been minimized.

@gadenbuie gadenbuie marked this pull request as ready for review April 4, 2024 21:57
@gadenbuie gadenbuie requested a review from cpsievert April 5, 2024 15:06
@gadenbuie gadenbuie requested a review from schloerke April 5, 2024 18:28
gadenbuie and others added 3 commits April 5, 2024 15:49
Comment on lines +41 to +47
def dep_shinyswatch_bootstrap_js() -> HTMLDependency:
return HTMLDependency(
name="shinyswatch-js",
version=bsw5_version,
source={"package": "shinyswatch", "subdir": bs5_path},
script={"src": "bootstrap.bundle.min.js"},
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only be included for shiny <= 0.8.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this PR still removes the Bootstrap JS dependency. We'll take that out in a follow up PR once shiny has some run-time Bootstrap compatibility checks. Then we'll require latest shiny in shinswatch.

@gadenbuie gadenbuie merged commit 52dfc4b into main Apr 10, 2024
14 checks passed
@gadenbuie gadenbuie deleted the refactor/decouple-shiny-bootstrap branch April 10, 2024 14:32
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

Successfully merging this pull request may close these issues.

None yet

2 participants