-
Notifications
You must be signed in to change notification settings - Fork 63
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 Kino.Download #174
Add Kino.Download #174
Conversation
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.
Hey @TylerPachal, thanks for the PR :) A couple comments, since we need a bit different approach.
lib/kino/download.ex
Outdated
""" | ||
export function init(ctx, data) { | ||
var hiddenElement = document.createElement('a'); | ||
hiddenElement.href = 'data:attachment/text,' + encodeURI(data.content); |
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.
To make it more generic, we can receive any binary from the server, then encode it as base64 and use data:application/octet-stream;base64;${dataBase64}
. If we need to customize the content type explicitly, we can make it an option in Elixir, but I think we should be fine with application/octet-stream
, especially that we specify the file name.
Co-authored-by: Jonatan Kłosko <[email protected]>
lib/kino/download.ex
Outdated
hiddenElement.click(); | ||
}); | ||
|
||
// Create the Download button |
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.
Annotation: Not sure if there is any styling we can add 😅
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.
Yeah, we can use any CSS we want, but don't worry about it, I can add some Livebook-matching style later :)
Co-authored-by: Jonatan Kłosko <[email protected]>
Co-authored-by: Jonatan Kłosko <[email protected]>
Co-authored-by: Jonatan Kłosko <[email protected]>
Co-authored-by: Jonatan Kłosko <[email protected]>
Co-authored-by: Jonatan Kłosko <[email protected]>
Co-authored-by: Jonatan Kłosko <[email protected]>
Co-authored-by: Jonatan Kłosko <[email protected]>
Thanks for your help & feedback Jonatan! I am going to leave this here for now while we wait on #171 |
Sounds good, thanks :) |
We have @josevalim wdyt? |
We should also add a label option. The text that we show should be: |
Attempts to address #19.
I am new to Kino, and not a Javascript exeprt, so I would appreciate some feedback on this.
The Javascript code was mostly taken from this jsfiddle.
I tested it in my local livebook instance like this: