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

Change express.layout to express.ui #904

Merged
merged 21 commits into from
Dec 21, 2023
Merged

Change express.layout to express.ui #904

merged 21 commits into from
Dec 21, 2023

Conversation

wch
Copy link
Collaborator

@wch wch commented Dec 13, 2023

This PR changes the name of express.layout to express.ui, as described in #854 (comment).

It also adds a test to make sure that all items from shiny.ui are accounted for in shiny.express.ui, and vice versa. For both of these modules, either a corresponding item must exist in both modules, or it must be listed explicitly in a variable _known_missing.

There are a few remaining API questions:

  • For some functions, like card_header(), it's not clear to me whether they should be context managers or regular functions.

  • shiny.ui.page_navbar() takes a named argument sidebar, which comes after the *args. The express version of this function also takes a named argument sidebar, but the only way to use it is with something like this, which is really awkward:

    s = shiny.ui.sidebar(....)
    with shiny.express.ui.page_navbar(sidebar=s):
        ....

    It would be much better if we could pass in a sidebar object just as a *arg, and it would discover it and place it correctly.

@cpsievert
Copy link
Collaborator

cpsievert commented Dec 13, 2023

@wch fwiw #893 addresses that sidebar issue for page_navbar(), meaning you can do something like this and it infers a page_navbar() context is relevant, and puts the sidebar in the keyword arg.

with ui.sidebar():
   ...

with ui.nav("Page 1"):
    "Page 1 content"

with ui.nav("Page 1"):
    "Page 2 content"

We still have that (sidebar kwarg) problem for navset_*(sidebar=) though, so it's maybe also worth having relevant navsets allow for a sidebar in their args

@schloerke schloerke modified the milestones: v0.7.0, v0.6.1 Dec 18, 2023
@wch wch requested a review from jcheng5 December 21, 2023 01:49
@jcheng5
Copy link
Collaborator

jcheng5 commented Dec 21, 2023

I would lean context manager for card_header, if I had to choose one. Would it be crazy to support both, like the tags do? I know we can't do this for containers that have nontrivial logic relating to their children.

if callable(getattr(res, "tagify", None)):
return res.tagify() # pyright: ignore
if callable(getattr(res, "_repr_html_", None)):
return res._repr_html_() # pyright: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if someday we could ask htmltools if an object is outputtable

@wch wch merged commit b6451fd into main Dec 21, 2023
25 checks passed
@wch wch deleted the express-layout-to-ui branch December 21, 2023 19:24
schloerke added a commit that referenced this pull request Jan 8, 2024
* 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
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants