Skip to content

Make sure the fill HTML dependency comes for the ride with page_fillable() #861

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Make sure the fill HTML dependency comes for the ride with page_fillable() #861

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions shiny/ui/_page.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
from ._tag import consolidate_attrs
from ._utils import get_window_title
from .css import CssUnit, as_css_padding, as_css_unit
from .fill._fill import FILLABLE_CONTAINTER_ATTRS
from .fill._fill import as_fillable_container


def page_sidebar(
Expand Down Expand Up @@ -298,13 +298,8 @@ def page_fillable(

style = css(padding=as_css_padding(padding), gap=as_css_unit(gap))

return page_bootstrap(
page = page_bootstrap(
head_content(tags.style("html { height: 100%; }")),
# Even though page_bootstrap accepts *args/**kwargs, we need to prepend the
# class value to the tags.body. To avoid having a <body> within a <body> for a
# core code path, we can manually use `FILLABLE_CONTAINER_ATTRS` here as the
# first set of attributes.
FILLABLE_CONTAINTER_ATTRS,
{"class": "bslib-page-fill bslib-gap-spacing", "style": style},
{"class": "bslib-flow-mobile"} if fillable_mobile else None,
attrs,
Expand All @@ -314,6 +309,15 @@ def page_fillable(
lang=lang,
)

# page returns a <html> tag, but we need to make the <body> fillable
body = page.children[1]
if not isinstance(body, Tag) or body.name != "body":
raise ValueError("Expected a <body> tag")

as_fillable_container(body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is being called for its side effect, modifying the object in place. I think in all other instances in the code base, we call it for its return value.

The function definition has a question of how it should work:

py-shiny/shiny/ui/fill/_fill.py

Lines 33 to 40 in ddfd762

@add_example()
# TODO-future-API; These methods mutate their input values. Should they return a new object instead? Or not return at all (like `LIST.sort()`)?
def as_fillable_container(
tag: TagT,
) -> TagT:
tag_prepend_class(tag, FILL_CONTAINER_CLASS)
tag.append(fill_dependency())
return tag

I think that for clarity and consistency, that function should only be used for its return value, and shouldn't alter the object in place. I'll make a follow-up PR that changes that function, and also alters the usage here to match.


return page


@add_example()
def page_fluid(
Expand Down