-
Notifications
You must be signed in to change notification settings - Fork 1.7k
scalars: use tf-downloader for download links
#1846
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
Conversation
Summary: We have a `<tf-downloader>` element that provides CSV and JSON download links alongside a run selector, but this element is actually unused. This commit replaces the scalar plugin’s reimplementation, updating the styling of `<tf-downloader>` to stay mostly consistent with the existing UI. This will enable us to fix #1845 in a way that can work for other download links, too. Test Plan: Observe that the UI is virtually identical (the only visible change is that the vertical position of the dropdown menu has shifted slightly):  Check that the download links download identical files before and after the change, with the same filenames. wchargin-branch: scalars-tf-downloader
nfelt
left a comment
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.
I always wondered why <tf-downloader> existed but wasn't actually used. Hopefully not because it has some fatal flaw.
| }, | ||
| _csvName(tag, run) { | ||
| return `run_${run},tag_${tag}.csv`; | ||
| return `run_${run}-tag-${tag}.csv`; |
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.
this uses an underscore for run but dashes for the rest - make it consistent? ditto below
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.
This was for consistency with the original scalar-card code
| download="run_[[_runToDownload]]-tag-[[tag]].csv" |
but I’m fine with disregarding that.
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.
Ah okay. I think we can just disregard that; I doubt anyone is depending on the exact format of that downloaded file name.
| runs: Array, | ||
| tag: String, | ||
| urlFn: Function, | ||
| urlFn: Function, // (tag: string, run: string) => string (JSON URL) |
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.
Maybe move this comment up one line and just slightly expand to state that the user should provide this to generate URLs for the selected run/tag combos.
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.
Sure.
wchargin-branch: scalars-tf-downloader
wchargin-branch: scalars-tf-downloader
Summary:
We have a
<tf-downloader>element that provides CSV and JSON downloadlinks alongside a run selector, but this element is actually unused.
This commit replaces the scalar plugin’s reimplementation, updating the
styling of
<tf-downloader>to stay mostly consistent with the existingUI. This will enable us to fix #1845 in a way that can work for other
download links, too.
Test Plan:
Observe that the UI is virtually identical (the only visible change is
that the vertical position of the dropdown menu has shifted slightly):
Check that the download links download identical files before and after
the change, with the same filenames except that the
run_prefix hasbeen changed to
run-for internal consistency.wchargin-branch: scalars-tf-downloader