-
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
Smarter, lazier, and more complete page default/api for express #893
Conversation
…fault. Gut layout.page_*() for now.
Things the user needs to control:
Possible interface for this:
|
I've added the following functions:
If the user wants to explicitly choose which page function is used, there are the following:
All of these functions live in |
shiny/express/_page.py
Outdated
if _fillable: | ||
_page_fn = ( | ||
ui.page_fillable | ||
) # pyright: ignore[reportGeneralTypeIssues] |
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.
Not that we absolutely need this now, but I think we could support wider=F
when fillable=T
by including a {"class": "container"}
(probably on the <body>
)?
From a conversation with @cpsievert, @gadenbuie, @jcheng5: Things we can do now:
Things we can do possibly in the future:
|
I've retargeted this PR against I implemented the changes as described in the previous comment:
|
shiny/ui/_page.py
Outdated
The page function to use. If ``None`` (the default), will automatically choose | ||
one based on the arguments provided. | ||
_fillable | ||
This has an effect only if there are no sidebars or top-level navs. If ``True``, |
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_navbar
and page_sidebar
both take a fillable
argument, we could pass it through
``False`` (the default), the value of ``_full_width`` will determine which page | ||
function is used. | ||
_full_width | ||
This has an effect only if there are no sidebars or top-level navs, and |
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.
Not as part of this PR, but we should support narrow layouts with sidebars or top-level navs.
_page_fn: Callable[..., Tag] | None = None, | ||
_fillable: bool = False, | ||
_full_width: bool = False, | ||
**kwargs: object, |
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 underscore prefixes, and make the **kwargs
a dictionary argument instead
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.
Or don't take _page_fn
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 considered taking out page_fn
, but I think we should keep it because it's useful when, in Express, you call page_opts(page_fn=...)
. The page_opts
function can just pass it through to this function.
94348ac
to
cdf8a3e
Compare
* main: (24 commits) Use dynamic version of py-shiny for deploy tests (#970) Add underscores to hide some imports (#978) Add rsconnect json files(shinyapps.io tests) and folium tests (#928) Express' `value_box()` no longer includes named positional args (#966) Include `tooltip()` and `popover()` in express (#949) Remove extra call to run_express() Call `tagify()` early to intercept `AttributeErrors` (#941) Don't pass sidebar twice to navbar_page Update changelog Update changelog Switch from `requests` to `urllib` (#940) Bump version to 0.6.1.1 Fix docstring for page_opts Fix API doc sections for Express Smarter, lazier, and more complete page default/api for express (#893) Change `express.layout` to `express.ui` (#904) Remove `@output` from examples (#790) feat: Allow for `App` `server=` to take `input` only (#920) Add fixes for type stub generation (#828) Move quarto express docs to bottom ...
This PR currently just introduces a
TopLevelRecallContextManager()
, which inspects it's args to determine a smarter page (i.e.,page_sidebar()
/page_navbar()
/something else) based on the presence of top-levellayout.sidebar()
/layout.nav()
/other components.This new approach isn't compatible with the more immediate/imperative
layout.set_page(layout.page_*())
API, so I'm removing it for now. This does bring up, however, more questions as to how to the page API (#866) should look. Originally I was thinking.set_page_*()
+.set_title()
would be sensible, but on second thought, aset_page_preferences()
is probably more ideal. There you could not only control pagekwargs
(e.g.,title
,lang
, etc), but also parameters related to default page container decision (e.g.,fillable
,wider
(i.e.,page_fluid()
vspage_fixed()
), etc))Also note that, we still currently have the issue of
layout.sidebar()
in a non-top-level context. I think, ideally, a non-top-levelRecallContextManager
could handle this by doing essentially the same asTopLevelRecallContextManager
(i.e.,wrapping
anySidebar
(s) into alayout_sidebar()
)