-
Notifications
You must be signed in to change notification settings - Fork 69
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
Allow page_sidebar
and page_navbar
to take sidebars as unnamed args
#942
base: main
Are you sure you want to change the base?
Conversation
*args | ||
UI elements. | ||
UI elements. One of the elements must be a sidebar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI elements. One of the elements must be a sidebar. | |
UI elements. One of the elements must be a :class:`shiny.ui.Sidebar`. |
"page_sidebar() requires one sidebar, but no sidebars were found in args. Use `ui.sidebar(...)` to create one." | ||
) | ||
elif len(sidebars) > 1: | ||
raise ValueError( | ||
"page_sidebar() requires exactly one sidebar, but more sidebars were found in args." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"page_sidebar() requires one sidebar, but no sidebars were found in args. Use `ui.sidebar(...)` to create one." | |
) | |
elif len(sidebars) > 1: | |
raise ValueError( | |
"page_sidebar() requires exactly one sidebar, but more sidebars were found in args." | |
"`page_sidebar()` requires one `Sidebar`, but no sidebars were found in `*args`. Use `ui.sidebar(...)` to create one." | |
) | |
elif len(sidebars) > 1: | |
raise ValueError( | |
"`page_sidebar()` requires exactly one `Sidebar`, but more sidebars were found in `*args`." |
"page_sidebar() requires exactly one sidebar, but more sidebars were found in args." | ||
) | ||
sidebar = sidebars[0] | ||
args = tuple([x for x in args if x not in sidebars]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove here. Update above.
args = tuple([x for x in args if x not in sidebars]) |
@@ -85,6 +83,19 @@ def page_sidebar( | |||
if isinstance(title, str): | |||
title = tags.h1(title, class_="bslib-page-title") | |||
|
|||
# Extract sidebar from args | |||
sidebars = [x for x in args if isinstance(x, Sidebar)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make both tuples right away (without having to convert from list)
sidebars = [x for x in args if isinstance(x, Sidebar)] | |
sidebars = (x for x in args if isinstance(x, Sidebar)) | |
args = (x for x in args if not isinstance(x, Sidebar)) |
"`sidebar=` is not a `Sidebar` instance. Use `ui.sidebar(...)` to create one." | ||
if sidebar is None: | ||
# Extract sidebar from args | ||
sidebars = [x for x in args if isinstance(x, Sidebar)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use tuple...
sidebars = [x for x in args if isinstance(x, Sidebar)] | |
sidebars = (x for x in args if isinstance(x, Sidebar)) |
sidebars = [x for x in args if isinstance(x, Sidebar)] | ||
if len(sidebars) == 1: | ||
sidebar = sidebars[0] | ||
args = tuple([x for x in args if x not in sidebars]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args = tuple([x for x in args if x not in sidebars]) | |
args = (x for x in args if not isinstance(x, Sidebar)) |
This closes #939.
I didn't do this for the
navset_
functions (likenavset_card_tab
) but I think they probably should have similar modifications.