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 #967

Closed
wants to merge 2 commits into from
Closed

Add render.download #967

wants to merge 2 commits into from

Conversation

wch
Copy link
Collaborator

@wch wch commented Jan 4, 2024

This PR adds render.download. This is a proof-of-concept; the final implementation will have to be done after #964 is merged, and will be significantly different. But the app-developer-facing API will be mostly the same.

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"

However, there is currently one limitation with this: it would be nice to be able to pass in a label for the button, but it doesn't work, because of where the function is defined for the button. @schloerke With the changes in #964, would it be possible to pass the label to the ui.download_button function?

_meta: TransformerMetadata,
_fn: DownloadHandler,
*,
button_label: Optional[TagChild] = None,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The button_label gets passed here, but it currently isn't used, and doesn't end up in the UI. It would need to somehow go to the call to download_button_wrapped().

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should work great in #964

Comment on lines +574 to +585
def download_button_wrapped(id: str) -> Tag:
return _ui.download_button(id, label=id)


@output_transformer(
default_ui=download_button_wrapped
) # pyright: ignore[reportGeneralTypeIssues]
async def DownloadTransformer(
_meta: TransformerMetadata,
_fn: DownloadHandler,
*,
button_label: Optional[TagChild] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would work today with:

Suggested change
def download_button_wrapped(id: str) -> Tag:
return _ui.download_button(id, label=id)
@output_transformer(
default_ui=download_button_wrapped
) # pyright: ignore[reportGeneralTypeIssues]
async def DownloadTransformer(
_meta: TransformerMetadata,
_fn: DownloadHandler,
*,
button_label: Optional[TagChild] = None,
@output_transformer(
default_ui=ui.download_button,
default_ui_passthrough_args=["label"],
) # pyright: ignore[reportGeneralTypeIssues]
async def DownloadTransformer(
_meta: TransformerMetadata,
_fn: DownloadHandler,
*,
label: Optional[TagChild] = "Download",

Definitely will be easier with Barret's new class-based approach.

@wch wch mentioned this pull request Jan 6, 2024
@wch
Copy link
Collaborator Author

wch commented Jan 6, 2024

Closing in favor of #977.

@wch wch closed this Jan 6, 2024
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