[EMB-137] Add loader to file-renderer (and TypeScripted it) #135
Conversation
| download: string; | ||
| width: string = this.width || '100%'; | ||
| height: string = this.height || '100%'; | ||
| allowfullscreen: boolean = this.allowfullscreen || true; |
There was a problem hiding this comment.
also, should be &&, so passing false isn't overwritten.
There was a problem hiding this comment.
I like camelCase here, but we should be careful about subtle API changes. Big breaking changes are easier to spot.
There was a problem hiding this comment.
Also, re: camelCase, this directly maps to iframe's allowfullscreen so maybe it should remain all lowercased?
| height: string = this.height || '100%'; | ||
| allowfullscreen: boolean = this.allowfullscreen || true; | ||
| version: string | null = this.version || null; | ||
| isLoading: boolean = this.isLoading || true; |
| get mfrUrl(this: FileRenderer) { | ||
| let download = `${this.get('download')}?direct&mode=render&initialWidth=766`; | ||
| if (this.get('version')) { | ||
| download += `&version=${this.get('version')}`; |
There was a problem hiding this comment.
Is TS smart enough to know that this.get('version') isn't null here? Or does that only work for normal variables?
There was a problem hiding this comment.
It appears to not. Even though it does know that this.get('version') is string | null, the type guard doesn't seem to guarantee that it's just string. Maybe because the .get() could potentially return something different on the second call?
| {{#if download}} | ||
| <iframe src={{mfrUrl}} width={{width}} title={{t 'file_detail.mfr_iframe_title'}} height={{height}} scrolling="yes" marginheight="0" frameborder="0" allowfullscreen={{allowfullscreen}}> | ||
| {{#if isLoading}} | ||
| <div class='Loader ball-scale ball-dark'><div></div></div> |
There was a problem hiding this comment.
Would be nice to use {{loading-indicator}}, if we can.
There was a problem hiding this comment.
lol, I forgot we had a component for that
| width: string = this.width || '100%'; | ||
| height: string = this.height || '100%'; | ||
| allowfullscreen: boolean = this.allowfullscreen || true; | ||
| version: string | null = this.version || null; |
There was a problem hiding this comment.
I forget, can a file's version be 0?
jamescdavis
force-pushed
the
EMB-137_quick_files_Add_loading_indicator_to_file_detail_page
branch
5 times, most recently
from
73dee7e to
8980492
Compare
jamescdavis
force-pushed
the
EMB-137_quick_files_Add_loading_indicator_to_file_detail_page
branch
from
8980492 to
5cdabb0
Compare
jamescdavis
deleted the
EMB-137_quick_files_Add_loading_indicator_to_file_detail_page
branch
4 participants
Purpose
Add a loading indicator to the file-renderer (used on the file detail page).
Summary of Changes
file-renderercomponent to TypeScript w/ decoratorsSide Effects / Testing Notes
Use a large file to see the loader.
Ticket
https://openscience.atlassian.net/browse/EMB-137
Reviewer Checklist
CHANGELOG.md