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

Add render.download, take 2 #977

Merged
merged 7 commits into from
Jan 10, 2024
Merged

Add render.download, take 2 #977

merged 7 commits into from
Jan 10, 2024

Conversation

wch
Copy link
Collaborator

@wch wch commented Jan 6, 2024

This PR adds render.download, and supersedes #967.

The existing API is to use session.download, like this:

# UI contains:
ui.download_button("dl", "Download File")

# Server function contains:
@session.download(filename="hello.txt")
def dl():
    yield "Hello, world!\n"

This is quite a bit different from all other outputs, because it relies on the session object. This also doesn't work for Express apps, because there isn't a session object available. (I'll note here that it would be possible to add a session object that could be imported from shiny.express, but the change in this PR is a better one.)

With this PR, the app uses render.download instead of session.download:

# UI contains:
ui.download_button("dl", "Download File")

# Server function contains:
@render.download(filename="hello.txt")
def dl():
    yield "Hello, world!\n"

I think this makes more sense from the app author's perspective.

In an Express app, the code would look like this (the button is automatically inserted in the UI):

@render.download(filename="hello.txt")
def dl():
    yield "Hello, world!\n"

With Express, the label can also be set with label:

@render.download(filename="hello.txt", label="Download!")
def dl():
    yield "Hello, world!\n"

Currently, label is the only parameter for download_button that's exposed in render.download. There are others, like icon and class_ which are relatively commonly used -- maybe it's best to wait until we resolve #971 before we decide what to do with these other parameters.

@wch wch changed the base branch from main to output_transformer_class January 6, 2024 03:58
@wch wch requested a review from schloerke January 6, 2024 03:58
@wch wch added this to the v0.7.0 milestone Jan 6, 2024
This was referenced Jan 6, 2024
Base automatically changed from output_transformer_class to main January 9, 2024 22:14
@wch wch force-pushed the render-download2 branch 2 times, most recently from ee4bf1b to 4ddeb6c Compare January 10, 2024 17:58
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

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

LGTM! (given cosmetic tweaks)

shiny/api-examples/download/app.py Show resolved Hide resolved
shiny/api-examples/download/app.py Show resolved Hide resolved
shiny/api-examples/download_button/app.py Show resolved Hide resolved
shiny/api-examples/download_button/app.py Show resolved Hide resolved
shiny/api-examples/download_link/app.py Show resolved Hide resolved
shiny/api-examples/download_link/app.py Show resolved Hide resolved
shiny/render/_render.py Outdated Show resolved Hide resolved
shiny/session/_session.py Outdated Show resolved Hide resolved
shiny/ui/_download_button.py Outdated Show resolved Hide resolved
shiny/ui/_download_button.py Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator Author

wch commented Jan 10, 2024

Going to hold the output and Session removal for later, since those are already covered by #793.

@wch wch merged commit 05de458 into main Jan 10, 2024
24 checks passed
@wch wch deleted the render-download2 branch January 10, 2024 21:40
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.

Consider replacing output_args()
2 participants