-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ui: glimmerize download button component #18292
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.
Nice work thanks for this! ✨ I left a few comments for consideration but nothing blocking.
extension: 'txt', | ||
stringify: false, | ||
}); | ||
// TODO refactor and call service instead |
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.
Would this be the DownloadCsvService
?
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.
Yep! Chelsea is refactoring the service now to be more general. I'll then update this action to use the service when I use this component to implement downloading a generated PKI key
@action | ||
handleDownload() { | ||
const { document, URL } = window; | ||
const downloadElement = document.createElement('a'); | ||
downloadElement.download = this.filename; | ||
downloadElement.href = URL.createObjectURL(this.content); | ||
document.body.appendChild(downloadElement); | ||
downloadElement.click(); | ||
URL.revokeObjectURL(downloadElement.href); | ||
downloadElement.remove(); | ||
} |
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.
Quite a bit of code was refactored in this component beyond glimmerizing but it looks more straightforward and I pulled down the branch and downloads work as expected so it looks good to me!
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.
Thank you for testing locally! 🎉
One follow up, it might be nice to have some test coverage for this component. |
Yes! I started writing tests and then decided to wait until utilizing the service. If the service doesn't have enough test coverage then I'll fill that out. |
* initial glimmerization, delete toolbar-download-button component * remove extra line * cleanup component file * add data getter * delete toolbar download button component * add jsdoc to component * move class att directly to component, remove arg * remove content getter
Glimmerizes the
DownloadButton
component and removesToolbarDownloadButton
This PR also removes checking for
window.navigator.msSaveOrOpenBlob
added by #4376 because IE and Edge didn't like the File constructor. Edge added support forFile API
in 2020 so we no longer need this conditional! 🎉(P.s. we no longer support IE)
class="toolbar-link"
class="button is-ghost"