Skip to content
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.

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

page_sidebar and page_navbar should drop sidebar parameter and accept Sidebar objects in *args #939

Open
wch opened this issue Dec 22, 2023 · 5 comments · May be fixed by #942
Open

page_sidebar and page_navbar should drop sidebar parameter and accept Sidebar objects in *args #939

wch opened this issue Dec 22, 2023 · 5 comments · May be fixed by #942
Labels

Comments

@wch
Copy link
Collaborator

wch commented Dec 22, 2023

Currently, page_sidebar() and page_navbar() take an explicit sidebar argument.

However, for page_auto(), which is used in Express mode, there's no sidebar argument (because children can't be passed as named arguments in Express). Instead, we inspect the *args, pull out any Sidebar objects, and then explicitly pass them to the sidebar argument.

py-shiny/shiny/ui/_page.py

Lines 529 to 530 in a020af6

page_fn = page_sidebar # pyright: ignore[reportGeneralTypeIssues]
args = tuple(sidebars + [x for x in args if x not in sidebars])

Also, because of how Python handles named and unnamed ages:

  • For page_sidebar(), the signature is page_sidebar(sidebar, *args). This means that the sidebar must be the first argument and must not be named.
  • For page_navbar(), the signature is page_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 for Sidebar objects -- essentially, take the logic from page_auto() and move it into page_sidebar() and page_navbar().

@wch wch added this to the v0.7.0 milestone Dec 22, 2023
@wch wch added the express label Dec 22, 2023
@schloerke
Copy link
Collaborator

take the logic from page_auto() and move it into page_sidebar() and page_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.)

@wch
Copy link
Collaborator Author

wch commented Jan 3, 2024

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:

  • For page_sidebar(): In Express, a Sidebar can go anywhere, but in Core, it must go first.
  • For 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 is page_sidebar(sidebar, *args). This means that the sidebar must be the first argument and must not be named.
  • For page_navbar(), the signature is page_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.

@jcheng5
Copy link
Collaborator

jcheng5 commented Jan 3, 2024

Couple of thoughts:

  1. Why would an Express user expect Core's page_sidebar and page_navbar to work the same way as Express's default layout? Isn't that where page_auto would come in?
  2. What if we keep the 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.

@gshotwell gshotwell mentioned this issue Jan 4, 2024
55 tasks
@wch
Copy link
Collaborator Author

wch commented Jan 10, 2024

There are a number of open questions, so we're going to defer this for after the 0.7.0 release.

@wch wch removed this from the v0.7.0 milestone Jan 10, 2024
@gadenbuie
Copy link
Collaborator

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()
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants