Skip to content
Closed
Show file tree
Hide file tree
Changes from 7 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',
LoaderSmallCircle = 'assets/images/loader-small.gif',
LoaderLargeSquare = 'assets/images/loader-large.gif',
LoaderHorizontal = 'assets/images/loader-horizontal.gif'
}

export const enum LoaderTypes {
Copy link
Contributor

Choose a reason for hiding this comment

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

LoaderType

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

SmallCircle = 'small-circle',
Horizontal = 'horizontal',
LargeSquare = 'large-square'
Copy link
Contributor

Choose a reason for hiding this comment

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

Spoke to Michael.

Let's use smallCircle directly in Spinner. We can remove it from here

Horizontal can be renamed to ExpandableRow. LargeSquare can be renamed to 'Page'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we would need to bring back the old svg icon approach for other skeleton loaders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will update the names,

Do you mean to use the small-circle as default? Not sure what is meant by the Spinner.

One thing I want to highlight here there is no need to use both ht-icon and image tag. An image tag can also work with an SVG source path.

But if we put it back then we need to put more logic on what type of loader goes where and then computing the image path and icon path(in case there are multiple) will go in 2 totally separate directions. I would prefer to keep one kind of logic (either SVG or IMG tag) as it would be straightforward and won't create any confusion.

}
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';
36 changes: 24 additions & 12 deletions projects/components/src/load-async/load-async.directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ import {
TemplateRef,
ViewContainerRef
} from '@angular/core';
import { LoaderTypes } from '@hypertrace/assets-library';
import { Observable, ReplaySubject } from 'rxjs';
import { LoadAsyncContext, LoadAsyncService } from './load-async.service';
import {
LOADER_TYPE,
ASYNC_WRAPPER_PARAMETERS$,
LoadAsyncWrapperComponent,
LoadAsyncWrapperParameters
Expand All @@ -23,26 +25,19 @@ 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!: LoaderTypes;

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(
private readonly viewContainer: ViewContainerRef,
private readonly componentFactoryResolver: ComponentFactoryResolver,
private readonly loadAsyncService: LoadAsyncService,
public readonly templateRef: TemplateRef<LoadAsyncContext>
) {
this.wrapperInjector = Injector.create({
providers: [
{
provide: ASYNC_WRAPPER_PARAMETERS$,
useValue: this.wrapperParamsSubject.asObservable()
}
],
parent: this.viewContainer.injector
});
}
) {}

public ngOnChanges(): void {
if (this.data$) {
Expand All @@ -65,6 +60,23 @@ export class LoadAsyncDirective implements OnChanges, OnDestroy {
private buildWrapperView(): ComponentRef<LoadAsyncWrapperComponent> {
const componentFactory = this.componentFactoryResolver.resolveComponentFactory(LoadAsyncWrapperComponent);

// Second param for structural directive is undefined until this.data$ is defined
// So putting this assignment in constuctor will not work
// This will execute only once as this method is called only
this.wrapperInjector = Injector.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to create a new injector. It would be problematic and also it wouldn't work since right now we are only creating a new instance of component when data$ reference itself change. New emits on data$ is automatically passed via the state$ property.

What I would recommend it to modify line 45 instead and add loaderType$ as a property in LoadAsyncWrapperParameters.

Create a ReplaySubject for loaderType ->

private readonly loaderTypeSubject: BehaviourSubject<LoaderType> = new BehaviourSubject(<default-value>);
private readonly loaderType$: Observable<LoaderType>= this.loaderTypeSubject.asObservable();

Then use add loaderType$ on like 46.

and add a separate ngOnChanges trigger for loaderType, that would just emit on this subject.

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

providers: [
{
provide: ASYNC_WRAPPER_PARAMETERS$,
useValue: this.wrapperParamsSubject.asObservable()
},
{
provide: LOADER_TYPE,
useValue: this.htLoadAsyncLoaderType
}
],
parent: this.viewContainer.injector
});

return this.viewContainer.createComponent(componentFactory, undefined, this.wrapperInjector);
}
}
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;
}
}
33 changes: 28 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,38 @@
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, LoaderTypes } 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: LoaderTypes = LoaderTypes.Horizontal;
public imagePath: ImagesAssetPath = ImagesAssetPath.LoaderHorizontal;

public ngOnChanges(): void {
// Unfortunatly passing undefined is overwriting default value
this.type = this.type ?? LoaderTypes.Horizontal;
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be modifying the input property. From where, is it getting the undefined value?

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

this.imagePath = this.getImagePathFromType();
}

private getImagePathFromType(): ImagesAssetPath {
switch (this.type) {
case LoaderTypes.Horizontal:
return ImagesAssetPath.LoaderHorizontal;
case LoaderTypes.LargeSquare:
return ImagesAssetPath.LoaderLargeSquare;
case LoaderTypes.SmallCircle:
return ImagesAssetPath.LoaderSmallCircle;
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,5 +1,5 @@
import { ChangeDetectionStrategy, Component, Inject, InjectionToken, TemplateRef } from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { IconType, LoaderTypes } from '@hypertrace/assets-library';
import { Observable } from 'rxjs';
import { switchMap, tap } from 'rxjs/operators';
import { LoadAsyncStateType } from '../load-async-state.type';
Expand All @@ -9,12 +9,13 @@ export const ASYNC_WRAPPER_PARAMETERS$ = new InjectionToken<Observable<LoadAsync
'ASYNC_WRAPPER_PARAMETERS$'
);

export const LOADER_TYPE = new InjectionToken<string>('LOADER_TYPE');
@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>
<ht-loader [type]="this.loaderType"></ht-loader>
</ng-container>
<ng-container *ngSwitchCase="'${LoadAsyncStateType.Success}'">
<ng-container *ngTemplateOutlet="this.content; context: state.context"></ng-container>
Expand All @@ -36,15 +37,20 @@ export class LoadAsyncWrapperComponent {
public icon?: IconType;
public title?: string;
public description: string = '';
public loaderType: LoaderTypes;

public content?: TemplateRef<LoadAsyncContext>;

public constructor(@Inject(ASYNC_WRAPPER_PARAMETERS$) parameters$: Observable<LoadAsyncWrapperParameters>) {
public constructor(
@Inject(ASYNC_WRAPPER_PARAMETERS$) parameters$: Observable<LoadAsyncWrapperParameters>,
@Inject(LOADER_TYPE) loaderType: LoaderTypes
) {
this.state$ = parameters$.pipe(
tap(params => (this.content = params.content)),
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))
);
this.loaderType = loaderType;
}

private updateMessage(stateType: LoadAsyncStateType, description: string = ''): void {
Expand Down
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