-
Notifications
You must be signed in to change notification settings - Fork 11
feat: download json component #866
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
Changes from all commits
7f0dcae
89fd501
d5b3d3b
c72f0cb
10eb204
dfffe37
ff4a965
fff865b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
@import 'color-palette'; | ||
|
||
.download-json { | ||
width: 40px; | ||
height: 40px; | ||
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import { Renderer2 } from '@angular/core'; | ||
import { fakeAsync } from '@angular/core/testing'; | ||
import { RouterTestingModule } from '@angular/router/testing'; | ||
import { createHostFactory, mockProvider, Spectator } from '@ngneat/spectator/jest'; | ||
import { MockComponent } from 'ng-mocks'; | ||
import { Observable, of } from 'rxjs'; | ||
import { ButtonComponent } from '../button/button.component'; | ||
import { IconComponent } from '../icon/icon.component'; | ||
import { DownloadJsonComponent } from './download-json.component'; | ||
import { DownloadJsonModule } from './download-json.module'; | ||
|
||
describe('Button Component', () => { | ||
let spectator: Spectator<DownloadJsonComponent>; | ||
const mockElement = document.createElement('a'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will go away if we put the anchor into the template |
||
const createElementSpy = jest.fn().mockReturnValue(mockElement); | ||
|
||
const createHost = createHostFactory({ | ||
component: DownloadJsonComponent, | ||
imports: [DownloadJsonModule, RouterTestingModule], | ||
declarations: [MockComponent(ButtonComponent), MockComponent(IconComponent)], | ||
providers: [ | ||
mockProvider(Document, { | ||
createElement: createElementSpy | ||
}), | ||
mockProvider(Renderer2, { | ||
setAttribute: jest.fn() | ||
}) | ||
], | ||
shallow: true | ||
}); | ||
|
||
const dataSource$: Observable<unknown> = of({ | ||
spans: [] | ||
}); | ||
|
||
test('should have only download button, when data is not loading', () => { | ||
spectator = createHost(`<ht-download-json [dataSource]="dataSource"></ht-download-json>`, { | ||
hostProps: { | ||
dataSource: dataSource$ | ||
} | ||
}); | ||
|
||
expect(spectator.query(ButtonComponent)).toExist(); | ||
}); | ||
|
||
test('should download json file', fakeAsync(() => { | ||
spectator = createHost(`<ht-download-json [dataSource]="dataSource"></ht-download-json>`, { | ||
hostProps: { | ||
dataSource: dataSource$ | ||
} | ||
}); | ||
|
||
spyOn(spectator.component, 'triggerDownload'); | ||
|
||
expect(spectator.component.dataLoading).toBe(false); | ||
expect(spectator.component.fileName).toBe('download'); | ||
expect(spectator.component.tooltip).toBe('Download Json'); | ||
const element = spectator.query('.download-json'); | ||
expect(element).toExist(); | ||
|
||
spectator.click(element!); | ||
spectator.tick(); | ||
|
||
expect(spectator.component.triggerDownload).toHaveBeenCalledTimes(1); | ||
})); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import { DOCUMENT } from '@angular/common'; | ||
import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, Input, Renderer2 } from '@angular/core'; | ||
import { IconType } from '@hypertrace/assets-library'; | ||
import { IconSize } from '@hypertrace/components'; | ||
import { Observable } from 'rxjs'; | ||
import { catchError, finalize, take } from 'rxjs/operators'; | ||
import { ButtonSize, ButtonStyle } from '../button/button'; | ||
import { NotificationService } from '../notification/notification.service'; | ||
|
||
@Component({ | ||
selector: 'ht-download-json', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would still like to see this component be renamed as it has nothing json-specific. |
||
changeDetection: ChangeDetectionStrategy.OnPush, | ||
styleUrls: ['./download-json.component.scss'], | ||
template: ` | ||
<div class="download-json" [htTooltip]="this.tooltip" (click)="this.triggerDownload()"> | ||
<ht-button | ||
*ngIf="!this.dataLoading" | ||
class="download-button" | ||
icon="${IconType.Download}" | ||
display="${ButtonStyle.Text}" | ||
size="${ButtonSize.Large}" | ||
></ht-button> | ||
<ht-icon *ngIf="this.dataLoading" icon="${IconType.Loading}" size="${IconSize.Large}"></ht-icon> | ||
</div> | ||
` | ||
}) | ||
export class DownloadJsonComponent { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like it could be easily renamed to be a generic download component There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As of now, this component does not support downloading other 'resources' like files etc. It only supports downloading a JavaScript data object as a JSON. Either we take care of all cases or rename this component to be more specific to its use case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It currently takes any string or object and supports downloading it as a file, which is the only download we'll likely have since we're using GQL. If we get new use cases in the future, can always expand supported inputs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, i was thinking we can build a wrapper around this, which we can call 'ht-download' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary, IMO. This is the only download mechanism we'll need, the only issue is that it won't always be json and that's already supported in the implementation as is. |
||
@Input() | ||
public dataSource!: Observable<unknown>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Data as an observable seems weird to me. This has potential side effects too. For example, If I pass a GraphQl query response observable and you subscribe on it (like you are doing below) it will fire a new query. Why not just use the data directly? You can do
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that better strictly from an api perspective, but from a functional one the issue there is it's going to force a download every time we show an icon, which we don't want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With respect to exporting Trace as Json feature, we can either optimize on space by fetching only when user clicks on the button or we can optimize on time by fetching in advance utilizing the idle browser/network/server cycle (in the background) In the former case, the download prompt will probably start after 2-3 sec (or more) depending upon the query. In the latter case, we are only using an idle browser and network cycle and download is immediate. With my suggestion, there are two problems:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That being said, I am fine with downloading on click time. The only concern I have is to avoid any unintended network call with any subscription inside this component. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we address that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Every network call we make, while yes async, will slow down others that are active, right? making 3 calls in parallel is faster than making 4. The tubes are only so big!
Future use cases presumably may have to fetch the data needed to download, too. Some cases it may be downloading the same data that's already displayed, but certainly case by case.
We could put it there with a loader, but those cases are really places where we need to load. For example, dropdowns, data panels etc. that are all reliant on data even if you don't interact with them.
I'm not positive what you mean by this, but if you mean the fact that it would make a new network call on each click, that's not necessarily wrong and is again how almost every download button in the world works - if I download something once, then click the download button again, it will just try to download it again (another network call). That said, the component's parent can easily decide by sharing or not sharing a passed in observable whether it wants that behavior or not - the beauty of observables! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There is not much UI rendering involved with this call. If we profile the page, i am confident the overall UI response time would only go down fractionally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
refer to test Example, If we are using this on events table, I would share the same data$ with this component. Since there is a subscription from within this component, it would likely trigger a secondary network call. (Unless I remember to share the observable inside the parent component) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's not the UI rendering, it's the network bandwidth - you can only transmit so many bits, no matter how many calls you make.That said, I don't disagree that it's likely a minimal perceived impact - but still a waste of both client and server resources.
If the parent wants to use the same observable in two places but only fetch once, its on the parent to share it - no one else can make that decision. Taking in a sync string for example and the parent using an async pipe like you suggested above |
||
|
||
@Input() | ||
public fileName: string = 'download'; | ||
|
||
@Input() | ||
public tooltip: string = 'Download Json'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove tooltip as indicated on the other PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would do in other PR |
||
|
||
public dataLoading: boolean = false; | ||
private readonly dlJsonAnchorElement: HTMLAnchorElement; | ||
|
||
public constructor( | ||
@Inject(DOCUMENT) private readonly document: Document, | ||
private readonly renderer: Renderer2, | ||
private readonly changeDetector: ChangeDetectorRef, | ||
private readonly notificationService: NotificationService | ||
) { | ||
this.dlJsonAnchorElement = this.document.createElement('a'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not make this an anchor in the template? could we wrap the button in an anchor so the user is actually clicking the anchor they're triggering? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since here we're using an observable to fetch the data, so it is not possible to do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if we don't wrap it (which I still think is doable), having an anchor in this template that's not displayed is more or less equivalent to what we're doing here, so is definitely feasible (basically just changes the createElement to populate that to a |
||
} | ||
|
||
public triggerDownload(): void { | ||
this.dataLoading = true; | ||
this.dataSource | ||
.pipe( | ||
take(1), | ||
catchError(() => this.notificationService.createFailureToast('Download failed')), | ||
finalize(() => { | ||
this.dataLoading = false; | ||
this.changeDetector.detectChanges(); | ||
}) | ||
) | ||
.subscribe((data: unknown) => { | ||
if (typeof data === 'string') { | ||
this.downloadData(data); | ||
} else { | ||
this.downloadData(JSON.stringify(data)); | ||
} | ||
}); | ||
} | ||
|
||
private downloadData(data: string): void { | ||
this.renderer.setAttribute( | ||
this.dlJsonAnchorElement, | ||
'href', | ||
`data:text/json;charset=utf-8,${encodeURIComponent(data)}` | ||
); | ||
this.renderer.setAttribute(this.dlJsonAnchorElement, 'download', `${this.fileName}.json`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please take the extension in as part of the file name as indicated on the other PR |
||
this.renderer.setAttribute(this.dlJsonAnchorElement, 'display', 'none'); | ||
this.dlJsonAnchorElement.click(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { CommonModule } from '@angular/common'; | ||
import { NgModule } from '@angular/core'; | ||
import { ButtonModule } from '../button/button.module'; | ||
import { IconModule } from '../icon/icon.module'; | ||
import { NotificationModule } from '../notification/notification.module'; | ||
import { TooltipModule } from '../tooltip/tooltip.module'; | ||
import { DownloadJsonComponent } from './download-json.component'; | ||
|
||
@NgModule({ | ||
declarations: [DownloadJsonComponent], | ||
imports: [CommonModule, ButtonModule, NotificationModule, IconModule, TooltipModule], | ||
exports: [DownloadJsonComponent] | ||
}) | ||
export class DownloadJsonModule {} |
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.
please update test describe name