Allow users or third-party packages to replace Bootstrap CSS #1270

gadenbuie · 2024-04-01T17:26:51Z

In Shiny for R, all page functions take a theme argument, with the following behavior:

If NULL, use Bootstrap 3
If a scalar string, assume it's a client-side path to a CSS file and add a <link> element to the page <head>. Note that base Bootstrap 3 is still included in this case.
If a bslib theme, add bslib::bs_theme_dependencies(theme) but not the base Bootstrap dependency after registering the theme (so that it can be used for runtime Sass compilation).

I'm proposing the following approach to theme in Python:

theme=MISSING ~~or theme=ui.theme()~~ default case
- Uses the default Shiny Bootstrap theme.
theme: str | Path
- Uses ui.include_css(theme) to include the CSS file. Shiny's Bootstrap bundle is still added, but without including the Bootstrap stylesheet.
```
theme=ui.theme(
    theme: str | HTMLDependency | Tagifiable,
    bs_version: Optional[str],
    replace = "css" | "all" | "none"
)
```
- Allows a CSS file, HTML dependency or a tagifiable that returns an HTML dependency to provide the theme.
- Includes optional bs_version that can be used for runtime checks to warn if the provided Bootstrap theme is out-of-date with Shiny's provided Bootstrap version.
- Allows theme providers to determine if the theme replaces the "css" of Shiny's Bootstrap bundle, the entire ("all") Bootstrap bundle, or the theme is overlaid over base Bootstrap ("none").
For completeness, theme could also be a dict with keys theme, bs_version, replace such that ui.theme(**dict) returns a full theme object.

The first two cases can be seen as a short-cut for providing ui.theme(), which itself returns a Theme or ShinyTheme instance.

The text was updated successfully, but these errors were encountered:

cpsievert · 2024-04-03T17:47:31Z

I feel uneasy about having an exported shiny.ui.theme() for what is mainly an advanced user / developer 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'm starting to question whether we need theme argument at all for this use case. Could we instead 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) -> tuple[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"}
        )
    ]

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 a bootstrap_js() if need be down the road. In general though, the thought of encouraging users to customize the JS gives me the heebie jeebies.

gadenbuie · 2024-04-03T18:25:30Z

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.

In this case, I agree, we wouldn't want to add a theme argument. For naming, I think ui.include_bootstrap_css() would fit in well. I'd also prefer to use a property on the HTMLDependency to report/check Bootstrap version, but we could fall back to parsing the banner in the CSS file if needed.

gadenbuie · 2024-04-03T21:21:14Z

#1282 shows what include_bootstrap_css() would look like.

@cpsievert can you take a look at the approach? This iteration has a bit more moving pieces, e.g. changes in shinyswatch are required posit-dev/py-shinyswatch#32, and it'd be good to hear your opinion before I get too far down the road.

Note that we don't need to make include_bootstrap_css() public, either. We do get runtime bootstrap version checking from this approach, but we could either keep the function internal or remove it in favor of expert users following the underlying mechanics.

gadenbuie · 2024-04-22T19:38:46Z

Coming back to this after a bit, I'm not seeing a clear winner between the two proposals.

feat: Add ui.theme() and theme argument to page_*() functions #1275 implements a theme argument as described in the OP, an abstraction allowing users to provide a path to a CSS file, an HTMLDependency with Bootstrap CSS, or a complete replacement for all of Shiny's Bootstrap dependency. This approach was targeted: theme affects the Bootstrap dependency Shiny attaches to the page.
feat: Add include_bootstrap_css() #1282 implements include_bootstrap_css() as proposed in Carson's comment above. This approach is decoupled: shiny.ui.page_*() always add the built-in Bootstrap bundle, but include_bootstrap_css() would help construct the right HTMLDependency to add the new CSS and suppress the built-in bundle.

To summarize the current state of affairs, the core work to be done is to separate the Bootstrap dependencies into CSS and JS components. Having done that, third-party theme providers can focus on providing only the Bootstrap CSS (and possibly on suppressing the built-in CSS). In other words, at a basic level, everything we need can be accomplished via htmltools dependency handling.

On the other hand, a htmltools-dependency-only approach pushes run-time Bootstrap compatibility checks into third-party packages. I think it would be better for most of the abstractions to live in py-shiny instead so they can be re-used and so that we have more control over their implementation in the long run.

As pointed out in https://github.com/posit-dev/py-shiny/pull/1282/files#r1552096813 the htmltools-dependency-only approach also needs to account for multiple calls to a function like include_bootstrap_css(). OTOH, using a theme argument nicely captures the uniqueness of the Bootstrap theme and nicely ties it to the page_*() function.

We could certainly do a mixture of both -- which would mean adding a theme argument to the page_*() functions (along the lines of #1275) while internally using an approach closer to that in #1282.

If we take the mixed approach, though, we'll need to agree on the shape of the data passed into theme. In this light, the goal would be to define an a contract that could be used by packages like shinyswatch and others to provide Bootstrap themes. This would give us the flexibility for internal modifications in py-shiny, as long as we maintain the contract, but it would require us to settle on key constraints now.

gadenbuie · 2024-04-23T17:10:02Z

Another, radically more simple option from @cpsievert: the theme argument only takes a path to a CSS file that is used to replace the Bootstrap CSS.

gadenbuie · 2024-04-23T18:05:00Z

It'd be nice to have an HTMLDependencyPartial that partially replaces a dependency; the idea would be to allow a package like shinyswatch to declare a partial dependency, e.g. a bootstrap.min.css that should only replace the Bootstrap dependency.

gadenbuie mentioned this issue Apr 2, 2024

feat: Add ui.theme() and theme argument to page_*() functions #1275

Closed

gadenbuie changed the title ~~Add theme argument to page_*() functions~~ Allow users or third-party packages to replace Bootstrap CSS Apr 3, 2024

gadenbuie mentioned this issue Apr 3, 2024

feat: Add include_bootstrap_css() #1282

Closed

gadenbuie mentioned this issue Apr 24, 2024

feat: Add theme argument to page functions #1334

Merged

gadenbuie closed this as completed in #1334 Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users or third-party packages to replace Bootstrap CSS #1270

Allow users or third-party packages to replace Bootstrap CSS #1270

gadenbuie commented Apr 1, 2024 •

edited

Loading

cpsievert commented Apr 3, 2024 •

edited

Loading

gadenbuie commented Apr 3, 2024

gadenbuie commented Apr 3, 2024 •

edited

Loading

gadenbuie commented Apr 22, 2024 •

edited

Loading

gadenbuie commented Apr 23, 2024

gadenbuie commented Apr 23, 2024

Allow users or third-party packages to replace Bootstrap CSS #1270

Allow users or third-party packages to replace Bootstrap CSS #1270

Comments

gadenbuie commented Apr 1, 2024 • edited Loading

cpsievert commented Apr 3, 2024 • edited Loading

gadenbuie commented Apr 3, 2024

gadenbuie commented Apr 3, 2024 • edited Loading

gadenbuie commented Apr 22, 2024 • edited Loading

gadenbuie commented Apr 23, 2024

gadenbuie commented Apr 23, 2024

gadenbuie commented Apr 1, 2024 •

edited

Loading

cpsievert commented Apr 3, 2024 •

edited

Loading

gadenbuie commented Apr 3, 2024 •

edited

Loading

gadenbuie commented Apr 22, 2024 •

edited

Loading