Skip to content
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions projects/assets-library/src/images/image-type.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const enum ImagesAssetPath {
ErrorPage = 'assets/images/error-page.svg',
LoaderSpinner = 'assets/images/loader-spinner.gif',
LoaderPage = 'assets/images/loader-page.gif',
LoaderExpandableRow = 'assets/images/loader-expandable-row.gif'
}

export const enum LoaderType {
Spinner = 'small-circle',
ExpandableRow = 'horizontal',
Page = 'large-square'
}
1 change: 1 addition & 0 deletions projects/assets-library/src/public-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ export * from './icons/icon-type';
export * from './icons/icon-registry.service';
export * from './icons/icon-library.module';
export * from './icons/testing/icon-library-testing.module';
export * from './images/image-type';
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
TemplateRef,
ViewContainerRef
} from '@angular/core';
import { LoaderType } from '@hypertrace/assets-library';
import { Observable, ReplaySubject } from 'rxjs';
import { LoadAsyncContext, LoadAsyncService } from './load-async.service';
import {
Expand All @@ -23,8 +24,11 @@ import {
export class LoadAsyncDirective implements OnChanges, OnDestroy {
@Input('htLoadAsync')
public data$?: Observable<unknown>;
@Input()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Input('htLoadAsyncLoaderType')
public loaderType: LoaderType;

This should work

public htLoadAsyncLoaderType!: LoaderType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be optional. You can assign a default value here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated


private readonly wrapperParamsSubject: ReplaySubject<LoadAsyncWrapperParameters> = new ReplaySubject(1);
private readonly wrapperInjector: Injector;
private wrapperInjector!: Injector;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for removing the readonly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

private wrapperView?: ComponentRef<LoadAsyncWrapperComponent>;

public constructor(
Expand All @@ -49,6 +53,7 @@ export class LoadAsyncDirective implements OnChanges, OnDestroy {
this.wrapperView = this.wrapperView || this.buildWrapperView();
this.wrapperParamsSubject.next({
state$: this.loadAsyncService.mapObservableState(this.data$),
loaderType: this.htLoadAsyncLoaderType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically, if htLoadAsyncLoaderType changes, then this if block will not be triggered? So, how would you handle the case when loaderType changes after init?

Why don't you send loaderType to mapObservableState as another param? then this function can add this loaderType in interface LoadingAsyncState { type: LoadAsyncStateType.Loading; }
Also, update L52 to also include change on LoaderType

content: this.templateRef
});
} else {
Expand Down
15 changes: 15 additions & 0 deletions projects/components/src/load-async/loader/loader.component.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,19 @@
flex-direction: column;
justify-content: center;
align-items: center;

.large-square {
height: 50px;
width: 50px;
}

.small-circle {
height: 20px;
width: 20px;
}

.horizontal {
height: 20px;
width: auto;
}
}
31 changes: 26 additions & 5 deletions projects/components/src/load-async/loader/loader.component.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,36 @@
import { ChangeDetectionStrategy, Component } from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { IconSize } from '../../icon/icon-size';
import { ChangeDetectionStrategy, Component, Input, OnChanges } from '@angular/core';
import { ImagesAssetPath, LoaderType } from '@hypertrace/assets-library';
import { assertUnreachable } from '@hypertrace/common';

@Component({
selector: 'ht-loader',
styleUrls: ['./loader.component.scss'],
template: `
<div class="ht-loader">
<ht-icon icon="${IconType.Loading}" size="${IconSize.Large}"></ht-icon>
<img [ngClass]="[this.type]" [src]="this.imagePath" />
</div>
`,
changeDetection: ChangeDetectionStrategy.OnPush
})
export class LoaderComponent {}
export class LoaderComponent implements OnChanges {
@Input()
public type: LoaderType = LoaderType.ExpandableRow;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

public imagePath: ImagesAssetPath = ImagesAssetPath.LoaderExpandableRow;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is formatting right? Can add a new line after L17

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


public ngOnChanges(): void {
this.imagePath = this.getImagePathFromType();
}

private getImagePathFromType(): ImagesAssetPath {
switch (this.type) {
case LoaderType.ExpandableRow:
return ImagesAssetPath.LoaderExpandableRow;
case LoaderType.Page:
return ImagesAssetPath.LoaderPage;
case LoaderType.Spinner:
return ImagesAssetPath.LoaderSpinner;
default:
return assertUnreachable(this.type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be we can leave type as optional and in default section here, we can assign the default image path instead of throwing exception

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, No. When a consumer is passing a wrong value it should throw an error, on the other hand, if the consumer is not passing a value it should show default value.

}
}
}
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
import { ChangeDetectionStrategy, Component, Inject, InjectionToken, TemplateRef } from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { Observable } from 'rxjs';
import { IconType, LoaderType } from '@hypertrace/assets-library';
import { BehaviorSubject, Observable } from 'rxjs';
import { switchMap, tap } from 'rxjs/operators';
import { LoadAsyncStateType } from '../load-async-state.type';
import { AsyncState, ErrorAsyncState, LoadAsyncContext } from '../load-async.service';

export const ASYNC_WRAPPER_PARAMETERS$ = new InjectionToken<Observable<LoadAsyncWrapperParameters>>(
'ASYNC_WRAPPER_PARAMETERS$'
);

@Component({
selector: 'ht-load-async-wrapper',
template: `
<div *ngIf="this.state$ | async as state" class="fill-container" [ngSwitch]="state.type">
<ng-container *ngSwitchCase="'${LoadAsyncStateType.Loading}'">
<ht-loader></ht-loader>
<ng-container *ngIf="this.loaderType$ | async as loaderType">
<ht-loader [type]="loaderType"></ht-loader>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ng-container is not needed. You can use async pipe directly with ht-loader

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do <ht-loader [type]="loaderType$ | async"></ht-loader>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loaderType here will be defined since we are setting default value.

</ng-container>
</ng-container>
<ng-container *ngSwitchCase="'${LoadAsyncStateType.Success}'">
<ng-container *ngTemplateOutlet="this.content; context: state.context"></ng-container>
Expand All @@ -39,9 +40,13 @@ export class LoadAsyncWrapperComponent {

public content?: TemplateRef<LoadAsyncContext>;

private readonly loaderTypeSubject: BehaviorSubject<LoaderType> = new BehaviorSubject<LoaderType>(LoaderType.Spinner);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are setting different default options at multiple places. It would be useful if we set it at only one place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then, I need to put undefined as the initial value.

public readonly loaderType$: Observable<LoaderType> = this.loaderTypeSubject.asObservable();

public constructor(@Inject(ASYNC_WRAPPER_PARAMETERS$) parameters$: Observable<LoadAsyncWrapperParameters>) {
this.state$ = parameters$.pipe(
tap(params => (this.content = params.content)),
tap(params => this.loaderTypeSubject.next(params.loaderType)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we gain anything by storing it as observable? If we store the loaderType similar to content, would it cause any issues?

switchMap(parameter => parameter.state$),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per prev comments, we can add loadertype as part of LoadingState. We wouldn't need to store another subject here. We can reference it from the state object directly.

tap(state => this.updateMessage(state.type, (state as Partial<ErrorAsyncState>).description))
);
Expand All @@ -65,5 +70,6 @@ export class LoadAsyncWrapperComponent {

export interface LoadAsyncWrapperParameters {
state$: Observable<AsyncState>;
loaderType: LoaderType;
content: TemplateRef<LoadAsyncContext>;
}
4 changes: 2 additions & 2 deletions projects/components/src/not-found/not-found.component.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import { ChangeDetectionStrategy, Component } from '@angular/core';
import { ImagesAssetPath } from '@hypertrace/assets-library';
import { NavigationService } from '@hypertrace/common';
import { ButtonRole, ButtonStyle } from '../button/button';

@Component({
selector: 'ht-not-found',
changeDetection: ChangeDetectionStrategy.OnPush,
styleUrls: ['./not-found.component.scss'],
template: `
<div class="not-found-container fill-container">
<div class="not-found-content">
<img class="not-found-image" src="assets/images/error-page.svg" loading="lazy" alt="not found page" />
<img class="not-found-image" src="${ImagesAssetPath.ErrorPage}" loading="lazy" alt="not found page" />
<div class="not-found-message-wrapper">
<div class="not-found-text-wrapper">
<div class="not-found-message">Page not found</div>
Expand Down