-
Notifications
You must be signed in to change notification settings - Fork 68
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
Sidebar improvements at smaller screen sizes #1436
Conversation
333b141
to
4d55656
Compare
shiny/ui/fill/_fill.py
Outdated
def add_fill_classes( | ||
tag: Tag, fill_item: bool = True, fillable_container: bool = True | ||
) -> Tag: | ||
if fill_item: | ||
tag.add_class(FILL_ITEM_CLASS) | ||
if fillable_container: | ||
tag.add_class(FILL_CONTAINER_CLASS) | ||
if fill_item or fillable_container: | ||
tag.append(fill_dependency()) | ||
return tag |
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.
With this being so close to the release, I'd rather use the existing as_fill_item()
/as_fillable_container()
.
There is also the subtle copy()
issue here, which I know isn't a problem in the way you're currently using it, but it'd be very easy for someone use this in future and unknowingly mutate user input
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.
I mean, the point is to avoid the unnecessary copy for elements we're creating internally. But OTOH I can see that it's worth avoiding the potential for mutating user input or having to document two separate approaches, plus it's only a shallow copy of the outer tag anyway. I'll go back to the existing functions.
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.
FTR, we actually make two copies because we need to call as_fill_item(as_fill_carrier(tag))
and both make a copy.
Personally, I'd definitely prefer what @schloerke recommended above: having add_fill_classes()
for internal usage and also calling it directly in as_fill_item()
and as_fill_carrier()
. I think a reasonable docstring on add_fill_classes()
would help with the mutating inputs issue.
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.
Anyway, 5bbe19e reverts back to using as_fill_item(as_fill_carrier(main))
Please update the title of this PR to reflect it bundles a hand-full of PRs |
Co-authored-by: Barret Schloerke <barret@posit.co>
Co-authored-by: Carson <cpsievert1@gmail.com>
Brings CSS and internal changes from rstudio/bslib#1057 and rstudio/bslib#1059 to Shiny for Python.
For testing the new behavior, use the Penguins dashboard in
examples/penguins/app.py
.Adds a
main.bslib-page-main
container around the main contents inui.page_sidebar()
andui.page_navbar(sidebar=ui.sidebar())
(navbar with a top-level shared sidebar).The
.bslib-page-main
class has min-width and min-height set to Bootstrap's small break point (576px). Min width/height are applied above the small breakpoint -- the goal is to prevent overly smooshing the main contents in the in-between sizes of a ShinyLive or IDE preview window that's bigger than a mobile screen width but still small enough that fillable layouts squish to become illegible.The min width/height are controllable via
--bslib-page-main-min-{width,height}
CSS variables or related$bslib-page-main-min-{width,height}
Sass variables (not yet applicable for Shiny for Python). Note that min width must be 576px or greater to have an affect.Currently, this min width/height is only applied for pages with a page-level sidebar. In the future we might apply this in other filling layouts as well.
This PR also brings
The biggest changes come from the first, which adds
always-above
as an option for mobile, e.g.ui.sidebar(open={"mobile": "always-above"})
, where the sidebar appears above the main content.