Skip to content

Conversation

@cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Dec 5, 2023

Closes #841
Closes #846

This PR does 3 things:

  1. Changes express' default page to page_fillable() (see Consider making page_fillable() the default for shiny.express #846 for motivation)
  2. Changes express.layout.sidebar() to be an interface to layout_sidebar(), not sidebar().
  3. In order for 2 to work sensibly, updates layout_sidebar() to accept a TagChild for it's first argument (sidebar) and wraps that value in a sidebar() if need be.

As a result of 2 and 3, we gain the following:

  • Avoid problems like Shiny express, placing UI elements above the sidebar context manager causes error. #841, where if with layout.sidebar() appears anywhere but the first top-level component, you'll get a error / undefined behavior.
  • Somewhat relatedly, we no longer need to special case the page layout for with layout.sidebar() in a meaningful way
    • Using a with layout.sidebar() as a single, top-level, component still fills the page sensibly
  • with layout.sidebar() now works sensibly as an inline sidebar layout, just like ui.layout_sidebar() does.

The main downside to this approach is that it may be surprising that code like this produces "Thing 1" in the sidebar, but "Thing 2"/"Thing 3" in the main region:

from shiny import ui
from shiny.express import layout

with layout.sidebar():
    ui.div("Thing 1")
    ui.div("Thing 2")
    ui.div("Thing 3")

And to place multiple elements in the sidebar, you need an explicit ui.sidebar()

from shiny import ui
from shiny.express import layout

with layout.sidebar():
    ui.sidebar(
        ui.div("Thing 1"), 
        ui.div("Thing 2")
    )
    ui.div("Thing 3")

That seems reasonable though, especially considering it seems as though we'll be needing this sort of pattern for other components, like cards:

from shiny import ui, render
from shiny.express import layout

with layout.card():
    ui.card_header("Title")
    @render.plot
     def foo():
          ...

…) equivalent to layout_sidebar() and let it's first arg be a non-sidebar object
@cpsievert cpsievert requested review from schloerke and wch December 5, 2023 23:08
@schloerke
Copy link
Collaborator

I like us pushing back on the assumption that shiny.express do not need to be implemented 1:1 with shiny.ui.

  1. Changes express.layout.sidebar() to be an interface to layout_sidebar(), not sidebar().

I disagree with this change, but I like it's intent of not requiring layout.layout_sidebar() for the top level.

I propose that layout.page_fillable() (or any layout method that can work with a sidebar, e.g. card() or layout_sidebar()) yank any top level Sidebar object to be implemented within ui.layout_sidebar() and all remaining top-level items be put into the main content. layout.layout_sidebar() would continue to exist as it does on main branch.

Ex:

from shiny import ui
from shiny.express import layout

ui.div("Thing 0")

with layout.sidebar():
    ui.div("Thing 1")
    ui.div("Thing 2")

ui.div("Thing 3")

with layout.card():
  ui.card_header("header content")
  with layout.sidebar():
    ui.div("sidebar content")
  ui.div("card main content")

... would produce a ui.page_sidebar() whose sidebar contains Thing 1 and Thing 2, and the main content has Thing 0, Thing 3, and the card (who has it's own sidebar of sidebar content) containing header content and card main content.

layout.card() could have sidebar=MISSING in **kwargs while still supporting it as a child. (Typing it out, I'd be happy even if we dropped any key-word sidebar parameter and only took it from an upgraded child.)

Assertions (for all methods who handle a sidebar) would need to be made that at most one sidebar object is supplied when upgrading the tags.

  1. In order for 2 to work sensibly, updates layout_sidebar() to accept a TagChild for it's first argument (sidebar) and wraps that value in a sidebar() if need be.

I believe this situation should also throw a warning / message if no Sidebar object is found in it's children. (And then as you said, wrap the first child in a sidebar().)

@cpsievert
Copy link
Collaborator Author

Thanks @schloerke. I'm coming back around to keeping layout.sidebar() essentially as-is (a global sidebar interface), so I'll likely be closing this, but splitting up some ideas here to smaller PRs

@cpsievert cpsievert changed the title Change express' default page to page_fillable(); make layout.sidebar(… Allow express' layout.sidebar to handle inline layouts Dec 6, 2023
@cpsievert
Copy link
Collaborator Author

Closing in favor of #847 (and #850).

@cpsievert cpsievert closed this Dec 6, 2023
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.

Consider making page_fillable() the default for shiny.express Shiny express, placing UI elements above the sidebar context manager causes error.

3 participants