Open wch opened 8 months ago
take the logic from
page_auto()
and move it intopage_sidebar()
andpage_navbar()
.
This motivation feels off to me as the API for Core is being driven by Express. (However, I am happy to have differences between Express and Core for usability!)
I'd like to keep as much static type checking as possible within Core. Because the sidebar
parameters are special and not interleaved within *args
children, I believe sidebar
argument should be exposed and typed.
I am ok with continuing to have page_auto
handle the sidebar differently than the two other page methods as its job is to auto detect the *args
objects. (If we don't like the API for page_sidebar()
or page_navbar()
and these proposed changes help improve the API, let me know.)
If we do keep this proposal, should we also apply it to layout_sidebar(sidebar, *args)
for consistency? (Or any other method that takes a sidebar
and set of tag children (*args
)?
@wch What about having shiny.express.ui.page_{sidebar, navbar}()
accept *args
and have them separate the args into Sidebar
and not Sidebar
args? This would fit the model of Express is different than Core where needed. (But not having the API be driven by Express at the cost of losing typing within Core.)
The motivation is in part to improve the Express API, but also to improve the Core API. That said, I think it can make sense to make changes to the Core API to reflect the Express API, if it provides a more consistent experience and learning ramp. If we were to keep the status quo, then there would be one way of structuring the UI code for Express, and a different way of structuring the code for Core:
page_sidebar()
: In Express, a Sidebar
can go anywhere, but in Core, it must go first.page_navbar()
: In Express, a Sidebar
can go anywhere (and is unnamed), but in Core, it must go last, and be a named argument.This means that the transition from Express to Core would require learning a new way of structuring the layout code.
There is also the inconsistency in the Core between thepage_sidebar()
and page_navbar()
API, which I mentioned earlier:
- For
page_sidebar()
, the signature ispage_sidebar(sidebar, *args)
. This means that the sidebar must be the first argument and must not be named.
- For
page_navbar()
, the signature ispage_navbar(*args, sidebar)
. This means that the sidebar must not be the first argument, and must be named.
The drawback to the proposed change is that in page_sidebar()
, there is no enforcement of requiring a Sidebar
component by the static type checker. But there still is a helpful runtime error.
Couple of thoughts:
page_sidebar
and page_navbar
to work the same way as Express's default layout? Isn't that where page_auto
would come in?page_sidebar
and page_navbar
signatures the way they are, but also sniff out the *args
? I don't even care if we document it. This way, the signatures for Core look sensible, while a naive port of the Express code to Core also does what that user would expect--except that page_sidebar
would still require the sidebar argument to be first. (Which I really don't see as a problem in Core; I think it's weird to have page_sidebar(output1, output2, sidebar)
and just ignore the sidebar's position in the list of arguments. I mean, fine if we happen to have that behavior, but I'd rather have a sensible function signature personally.)Longer-term, maybe it doesn't make sense to have page_sidebar
, page_navbar
, page_fluid
, page_fixed
, and page_fillable
in core--there's so much overlap between their capabilities.
There are a number of open questions, so we're going to defer this for after the 0.7.0 release.
Even just within Core, I think there's a strong argument to be made to align the function signatures of page_sidebar()
and page_navbar()
. It's relatively common to start with a page_sidebar()
and realize you need a navbar page, in which case you have to significantly rearrange your code. Allowing page_navbar()
to find a sidebar child in *args
would be smoother transition
# from
ui.page_sidebar(
ui.sidebar(),
*children
)
# to
ui.page_navbar(
ui.sidebar(),
ui.nav_panel(
*children
)
)
# instead of
ui.page_navbar(
ui.nav_panel(
*children
),
sidebar = ui.sidebar()
)
Currently,
page_sidebar()
andpage_navbar()
take an explicitsidebar
argument.However, for
page_auto()
, which is used in Express mode, there's nosidebar
argument (because children can't be passed as named arguments in Express). Instead, we inspect the*args
, pull out anySidebar
objects, and then explicitly pass them to thesidebar
argument.https://github.com/posit-dev/py-shiny/blob/a020af6e9428b7f6780697840434ea6574f3dcec/shiny/ui/_page.py#L529-L530
Also, because of how Python handles named and unnamed ages:
page_sidebar()
, the signature ispage_sidebar(sidebar, *args)
. This means that the sidebar must be the first argument and must not be named.page_navbar()
, the signature ispage_navbar(*args, sidebar)
. This means that the sidebar must not be the first argument, and must be named.I think we should remove the
sidebar
parameter from these functions and simply inspect the*args
forSidebar
objects -- essentially, take the logic frompage_auto()
and move it intopage_sidebar()
andpage_navbar()
.