diff --git a/CHANGELOG.md b/CHANGELOG.md index a20207b4c..0fea3804c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/shiny/api-examples/as_fill_item/app.py b/shiny/api-examples/as_fill_item/app.py index 70dfb98d1..20d0e748c 100644 --- a/shiny/api-examples/as_fill_item/app.py +++ b/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( diff --git a/shiny/api-examples/as_fillable_container/app.py b/shiny/api-examples/as_fillable_container/app.py index 47fd96208..a327f4d18 100644 --- a/shiny/api-examples/as_fillable_container/app.py +++ b/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)`")), diff --git a/shiny/experimental/ui/_deprecated.py b/shiny/experimental/ui/_deprecated.py index 9fb3f1601..aa50bb6dc 100644 --- a/shiny/experimental/ui/_deprecated.py +++ b/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: diff --git a/shiny/ui/_navs.py b/shiny/ui/_navs.py index f57338d76..35efadb76 100644 --- a/shiny/ui/_navs.py +++ b/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 diff --git a/shiny/ui/_page.py b/shiny/ui/_page.py index fc6026545..df3879373 100644 --- a/shiny/ui/_page.py +++ b/shiny/ui/_page.py @@ -314,7 +314,7 @@ def page_fillable( if not isinstance(body, Tag) or body.name != "body": raise ValueError("Expected a tag") - as_fillable_container(body) + page.children[1] = as_fillable_container(body) return page diff --git a/shiny/ui/_tag.py b/shiny/ui/_tag.py index f508c70d2..8ed5d2dca 100644 --- a/shiny/ui/_tag.py +++ b/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 diff --git a/shiny/ui/fill/_fill.py b/shiny/ui/fill/_fill.py index 87f58bd37..462300a23 100644 --- a/shiny/ui/fill/_fill.py +++ b/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 diff --git a/tests/pytest/test_navs.py b/tests/pytest/test_navs.py index 0bbce0813..4b35769f7 100644 --- a/tests/pytest/test_navs.py +++ b/tests/pytest/test_navs.py @@ -1,22 +1,23 @@ """Tests for """ +import contextlib import random import textwrap -from typing import Any, Callable +from typing import Generator from htmltools import TagList from shiny import ui from shiny._utils import private_seed -from shiny.ui._navs import NavSet # Fix the randomness of these functions to make the tests deterministic -def with_private_seed(func: Callable[[], NavSet], *args: Any, **kwargs: Any): +@contextlib.contextmanager +def private_seed_n(n: int = 0) -> Generator[None, None, None]: with private_seed(): - random.seed(0) - return func(*args, **kwargs) + random.seed(n) + yield def test_nav_markup(): @@ -32,7 +33,8 @@ def test_nav_markup(): ui.nav_control("Other item"), ) - x = with_private_seed(ui.navset_tab, a, b, ui.nav_control("Some item"), menu) + with private_seed_n(): + x = ui.navset_tab(a, b, ui.nav_control("Some item"), menu) assert TagList(x).render()["html"] == textwrap.dedent( """\ @@ -64,12 +66,8 @@ def test_nav_markup(): """ ) - x = with_private_seed( - ui.navset_pill, - menu, - a, - id="navset_pill_id", - ) + with private_seed_n(): + x = ui.navset_pill(menu, a, id="navset_pill_id") assert TagList(x).render()["html"] == textwrap.dedent( """\ @@ -96,17 +94,17 @@ def test_nav_markup(): """ ) - x = with_private_seed( - ui.navset_card_pill, - a, - ui.nav_menu("Menu", c), - b, - selected="c", - ) + with private_seed_n(): + x = ui.navset_card_pill( + a, + ui.nav_menu("Menu", c), + b, + selected="c", + ) assert TagList(x).render()["html"] == textwrap.dedent( """\ -
+
  • @@ -125,24 +123,24 @@ def test_nav_markup():
-
-
-
a
-
c
-
b
+
+
+
a
+
c
+
b
""" ) - x = with_private_seed( - ui.navset_bar, # type: ignore - ui.nav_menu("Menu", "Plain text", c), - title="Page title", - footer="Page footer", - header="Page header", - ) + with private_seed_n(): + x = ui.navset_bar( + ui.nav_menu("Menu", "Plain text", c), + title="Page title", + footer="Page footer", + header="Page header", + ) assert TagList(x).render()["html"] == textwrap.dedent( """\ @@ -164,10 +162,10 @@ def test_nav_markup():
-
+
Page header -
-
c
+
+
c
Page footer
"""