Download link function #128
Conversation
There was a problem hiding this comment.
Nice! Honestly quite a neat solution 😄
To be fair, I've just looked around all over to try to find a reference point for this, and I can't find one anywhere either. So given that, I think I agree with your comment about shinyGovstyle mainly being for styling, so with a few tweaks to the function documentation I'd say this PR could fully resolve #127.
One other thought I'd had when looking through is that there is another type of download pattern that we could account for - the radio button file type selection with a confirmation button to download, like how EES does it in the table tool? (example featured table, you'll see if you scroll to the bottom)
I have done a version of this in the provider dashboard in DfE, which mainly uses shinyGovstyle components, so interested to know if you think that's worth wrapping up in a bigger module in a separate PR, or if there's enough parts already we could just add an example of it in this PR to the showcase app with existing functions? Either way I think it'd be a pattern worth adding to this package to show how it should look (and allow us to easily test it in our upcoming accessibility audit)
I like the idea of the radio-download module, but instinct is that it should be a separate PR. Happy to do that work (especially as you've already done most of the leg-work in the provider dashboard anyway!), but think it's big / distinct enough it warrants it's own review process. |
There was a problem hiding this comment.
All looks great, raised #129 and added a comment on #127 for the extra things to look at in other PRs.
One final thing, as the file size thing still doesn't quite sit right, I think I've worked out what's missing - can we add a warning if the file_size argument is blank? That way if it is blank there's a very clear prompt to add it, and if anyone so desires they can ignore too (though really if they're using this it's probably because they care about best practices so they should expect to be nudged on them)
I've added a console warning now (and some tests). |
Overview of changes
Adding a standard way to show downloadable content in the form of
download_link(). I've based it heavily on a combination ofshiny::downloadLink()andshinyGovstyle::external_link(). As withshiny::downloadLink(), it works withshiny::downloadHandler(), but had the added options to explicitly provide the file type and file size.This is the resulting html when using
download_link():<a id="download_data" class="shiny-download-link shiny-bound-output" href="session/1492a1e83012ff241b242c018e6b28b6/download/download_data?w=" target="_blank" download="" aria-live="polite">Download data (CSV, 1 KB)</a>Which looks like this in the app (note the download_link is just the heading in the demo app):
Note that
aria-live="polite"is still set, but I think this is the right way to go. For example if the user set's the link_text or file_size to update automatically based on some dynamic selections, then we'd want that to be announced at the "the next graceful opportunity".I've not included a dynamic way to do the file sizes in this, but user's can just do their own workaround by using a reactive that evaluates a predicted file size (e.g.
table_size_in_mb <- reactive(...)) and then calling the download link asdownload_link("table_data", "Download table data", file_size = table_size_in_mb()). Admittedly the...in the above is doing a lot of heavy lifting in that description, but to be fair, shinyGovstyle is about styling rather than writing active code for user's, so maybe fulfilling that part would be overreach anyway.Fixes some of #127
PR Checklist
devtools::check()Reviewer instructions