Make as_fillable_container() and as_fill_item() not mutate their inputs (#862)

Author: wch

CHANGELOG.md

@@ -22,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * Update penguins example to credit Allison Horst and drop usage of `shiny.experimental` (#798).
 
+* `as_fillable_container()` and `as_fill_item()` no longer mutate the `Tag` object that was passed in. Instead, it returns a new `Tag` object. Also closed #856: these functions now put the `html-fill-container` and `html-fill-item` CSS classes last, instead of first. (#862)
+
 
 ## [0.6.0] - 2023-08-08
 

shiny/api-examples/as_fill_item/app.py

@@ -6,7 +6,7 @@
 from shiny.ui import fill
 
 
-def outer_inner() -> tuple[htmltools.Tag, htmltools.Tag]:
+def outer_inner() -> htmltools.Tag:
     inner = ui.div(
         id="inner",
         style=htmltools.css(
@@ -22,17 +22,17 @@ def outer_inner() -> tuple[htmltools.Tag, htmltools.Tag]:
             border="3px red solid",
         ),
     )
-    return outer, inner
+    return outer
 
 
-outer0, inner0 = outer_inner()
+outer0 = outer_inner()
 
-outer1, inner1 = outer_inner()
-fill.as_fill_item(inner1)
+outer1 = outer_inner()
+outer1.children[0] = fill.as_fill_item(outer1.children[0])
 
-outer2, inner2 = outer_inner()
-fill.as_fillable_container(outer2)
-fill.as_fill_item(inner2)
+outer2 = outer_inner()
+outer2 = fill.as_fillable_container(outer2)
+outer2.children[0] = fill.as_fill_item(outer2.children[0])
 
 
 app_ui = ui.page_fluid(

shiny/api-examples/as_fillable_container/app.py

@@ -6,7 +6,7 @@
 from shiny.ui import fill
 
 
-def outer_inner() -> tuple[htmltools.Tag, htmltools.Tag]:
+def outer_inner() -> htmltools.Tag:
     inner = ui.div(
         id="inner",
         style=htmltools.css(
@@ -22,23 +22,22 @@ def outer_inner() -> tuple[htmltools.Tag, htmltools.Tag]:
             border="3px red solid",
         ),
     )
-    return outer, inner
+    return outer
 
 
-outer0, inner0 = outer_inner()
-outer1, inner1 = outer_inner()
-outer2, inner2 = outer_inner()
+outer0 = outer_inner()
 
-fill.as_fillable_container(outer2)
-
-fill.as_fillable_container(outer2)
-fill.as_fill_item(inner2)
+outer1 = outer_inner()
+outer1 = fill.as_fillable_container(outer1)
 
+outer2 = outer_inner()
+outer2 = fill.as_fillable_container(outer2)
+outer2.children[0] = fill.as_fill_item(outer2.children[0])
 
 app_ui = ui.page_fluid(
     ui.markdown(
         """\
-        # `as_fill_container()`
+        # `as_fillable_container()`
 
         For an item to fill its parent element,
         * the item must have `as_fill_item()` be called on it
@@ -49,7 +48,7 @@ def outer_inner() -> tuple[htmltools.Tag, htmltools.Tag]:
     ),
     ui.row(
         ui.column(4, ui.h5("Default behavior")),
-        ui.column(4, ui.h5(ui.markdown("`as_fill_container(red)`"))),
+        ui.column(4, ui.h5(ui.markdown("`as_fillable_container(red)`"))),
         ui.column(
             4,
             ui.h5(ui.markdown("`as_fill_item(blue)` + `as_fillable_container(red)`")),

shiny/experimental/ui/_deprecated.py

@@ -922,7 +922,7 @@ def as_fill_carrier(
     warn_deprecated(
         "`shiny.experimental.ui.as_fill_carrier()` is deprecated. "
         "This method will be removed in a future version, "
-        "please use `shiny.ui.fill.as_fill_container()` and `shiny.ui.fill.as_fillable_item()` instead."
+        "please use `shiny.ui.fill.as_fillable_container()` and `shiny.ui.fill.as_fillable_item()` instead."
     )
 
     if min_height is not None:

shiny/ui/_navs.py

@@ -34,7 +34,6 @@
 from ._card import CardItem, WrapperCallable, card, card_body, card_footer, card_header
 from ._html_deps_shinyverse import components_dependency
 from ._sidebar import Sidebar, layout_sidebar
-from ._tag import tag_add_style
 from .css import CssUnit, as_css_padding, as_css_unit
 from .fill import as_fill_item, as_fillable_container
 
@@ -1129,7 +1128,7 @@ def _make_tabs_fillable(
     # must to be a fillable container.
     content = as_fillable_container(as_fill_item(content))
 
-    for child in content.children:
+    for i, child in enumerate(content.children):
         # Only work on Tags
         if not isinstance(child, Tag):
             continue
@@ -1146,9 +1145,11 @@ def _make_tabs_fillable(
             padding=as_css_padding(padding),
             __bslib_navbar_margin="0;" if navbar else None,
         )
-        child = tag_add_style(child, styles)
+        child.add_style(cast(str, styles))
         child = as_fillable_container(as_fill_item(child))
 
+        content.children[i] = child
+
     return content
 
 

shiny/ui/_page.py

@@ -314,7 +314,7 @@ def page_fillable(
     if not isinstance(body, Tag) or body.name != "body":
         raise ValueError("Expected a <body> tag")
 
-    as_fillable_container(body)
+    page.children[1] = as_fillable_container(body)
 
     return page
 

shiny/ui/_tag.py

@@ -2,7 +2,7 @@
 
 from typing import TypeVar, cast, overload
 
-from htmltools import Tag, TagAttrs, TagAttrValue, TagChild, div
+from htmltools import TagAttrs, TagAttrValue, TagChild, div
 
 TagChildT = TypeVar("TagChildT", bound=TagChild)
 
@@ -60,104 +60,3 @@ def trinary(x: bool | str | None) -> None | str:
         return "true"
     else:
         return "false"
-
-
-TagT = TypeVar("TagT", bound="Tag")
-
-# Tag.add_class(x: str) -> Self[Tag]:
-#     cls = self.attrs.get("class")
-#     if cls:
-#         x = cls + " " + x
-#     self.attrs["class"] = x
-#     return self
-
-
-# Do not export
-# "Prepend" version of `tag.add_class(class_)`
-def tag_prepend_class(tag: TagT, *class_: str | None) -> TagT:
-    classes = (
-        *class_,
-        tag.attrs.get("class"),
-    )
-    classes = [c for c in classes if c is not None]
-    if len(classes) == 0:
-        return tag
-    tag.attrs["class"] = " ".join(classes)
-    return tag
-
-
-def tag_remove_class(tag: TagT, *class_: str | None) -> TagT:
-    """
-    Remove a class value from the HTML class attribute.
-
-    Parameters
-    ----------
-    *class_
-        The class name to remove.
-
-    Returns
-    -------
-    :
-        The modified tag.
-    """
-    cls = tag.attrs.get("class")
-    # If no class values to remove from, quit
-    if not cls:
-        return tag
-
-    # Remove any `None` values
-    set_to_remove = {c for c in class_ if c is not None}
-
-    # If no classes to remove, quit
-    if len(set_to_remove) == 0:
-        return tag
-
-    # Get new set of classes
-    # Order matters, otherwise we could use `set()` subtraction: `set(cls.split(" ")) - set(class_)`
-    new_cls: list[str] = []
-    for cls_val in cls.split(" "):
-        if cls_val not in set_to_remove:
-            new_cls.append(cls_val)
-
-    # If no classes left, remove the attribute
-    if len(new_cls) == 0:
-        # If here, `attrs.class` must exist
-        tag.attrs.pop("class")
-        return tag
-
-    # Otherwise, set the new class
-    tag.attrs["class"] = " ".join(new_cls)
-    return tag
-
-
-def tag_add_style(
-    tag: TagT,
-    *style: str | None,
-) -> TagT:
-    """
-    Add a style value(s) to the HTML style attribute.
-
-    Parameters
-    ----------
-    *style
-        CSS properties and values already properly formatted. Each should already contain trailing semicolons.
-
-    See Also
-    --------
-    ~htmltools.css
-
-    Returns
-    -------
-    :
-        The modified tag.
-    """
-    styles = (
-        tag.attrs.get("style"),
-        *style,
-    )
-    non_none_style_tuple = (s for s in styles if s is not None)
-    style_str = "".join(non_none_style_tuple)
-
-    if style_str:
-        tag.attrs["style"] = style_str
-    return tag

shiny/ui/fill/_fill.py

@@ -1,12 +1,12 @@
 from __future__ import annotations
 
+from copy import copy
 from typing import TypeVar
 
 from htmltools import Tag, TagAttrs
 
 from ..._docstring import add_example
 from .._html_deps_shinyverse import fill_dependency
-from .._tag import tag_prepend_class, tag_remove_class
 
 __all__ = (
     "as_fillable_container",
@@ -31,17 +31,16 @@
 
 
 @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
+    res = copy(tag)
+    res.add_class(FILL_CONTAINER_CLASS)
+    res.append(fill_dependency())
+    return res
 
 
 @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_fill_item(
     tag: TagT,
 ) -> TagT:
@@ -64,17 +63,18 @@ def as_fill_item(
     Returns
     -------
     :
-        The original :class:`~htmltools.Tag` object (`tag`) with additional attributes
-        (and an :class:`~htmltools.HTMLDependency`).
+        A copy of the original :class:`~htmltools.Tag` object (`tag`) with additional
+        attributes (and an :class:`~htmltools.HTMLDependency`).
 
     See Also
     --------
     * :func:`~shiny.ui.fill.as_fillable_container`
     * :func:`~shiny.ui.fill.remove_all_fill`
     """
-    tag_prepend_class(tag, FILL_ITEM_CLASS)
-    tag.append(fill_dependency())
-    return tag
+    res = copy(tag)
+    res.add_class(FILL_ITEM_CLASS)
+    res.append(fill_dependency())
+    return res
 
 
 def remove_all_fill(
@@ -109,8 +109,8 @@ def remove_all_fill(
     * :func:`~shiny.ui.fill.as_fillable_container`
     """
 
-    tag_remove_class(tag, FILL_CONTAINER_CLASS)
-    tag_remove_class(tag, FILL_ITEM_CLASS)
+    tag.remove_class(FILL_CONTAINER_CLASS)
+    tag.remove_class(FILL_ITEM_CLASS)
     return tag