Skip to content

feat: Skeleton loader for LoadAsync directive #1373

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.

Sign up for GitHub

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

Merged
merged 30 commits into from
Jan 20, 2022
Merged

feat: Skeleton loader for LoadAsync directive #1373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
602cd22
feat: added skeleton component
Jan 13, 2022
3a3d19b
feat: added skeleton component
Jan 13, 2022
cba76a7
fix: integrating skeleton component with loader
Jan 13, 2022
e81d1df
style: adding loader types and fixing display
Jan 13, 2022
e879a4e
feat: added skeleton shapes and styling
Jan 14, 2022
24015a1
fix: linting errors
Jan 14, 2022
3ef1fe2
style: minor style adjustments to skeletons
Jan 14, 2022
5fd455a
feat: added donut skeleton shape
Jan 14, 2022
7f1cbcc
Merge branch 'main' into skeleton-loader
Jan 14, 2022
91a4832
fix: requested changes
Jan 14, 2022
bf2b846
fix: linting fixes
Jan 14, 2022
a6f5b7e
fix: default isOldLoaderFlag to true
Jan 16, 2022
1048339
fix: requested changes
Jan 16, 2022
3d048dd
test: updated for loader component changes
Jan 17, 2022
39ed6a3
Merge branch 'main' into skeleton-loader
Jan 17, 2022
dbdd5cf
fix: appeasing linter
Jan 17, 2022
6d90a76
test: skeleton component testing
Jan 17, 2022
d416769
Merge branch 'main' into skeleton-loader
Jan 17, 2022
91c7214
refactor: requested changes
Jan 18, 2022
17b08ee
fix: appeasing linter
Jan 18, 2022
86212a4
refactor: tests to account for requested changes
Jan 18, 2022
8f19a6d
refactor: some of requested changes
Jan 19, 2022
2e2048d
Merge branch 'main' into skeleton-loader
Jan 19, 2022
48418e0
refactor: to appease linter with switch
Jan 19, 2022
c99cebe
refactor: requested changes
Jan 19, 2022
429fc03
refactor: linter
Jan 19, 2022
12f0047
refactor: requested changes
Jan 19, 2022
6a07033
refactor: naming change
Jan 19, 2022
65a867e
refactor: name placement to after class definition
Jan 19, 2022
0b7c6b4
Merge branch 'main' into skeleton-loader
Jan 19, 2022
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
3 changes: 2 additions & 1 deletion projects/components/src/load-async/load-async.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { IconModule } from '../icon/icon.module';
import { MessageDisplayModule } from '../message-display/message-display.module';
import { SkeletonModule } from '../skeleton/skeleton.module';
import { LoadAsyncDirective } from './load-async.directive';
import { LoaderComponent } from './loader/loader.component';
import { LoadAsyncWrapperComponent } from './wrapper/load-async-wrapper.component';

@NgModule({
declarations: [LoadAsyncDirective, LoadAsyncWrapperComponent, LoaderComponent],
imports: [CommonModule, IconModule, MessageDisplayModule],
imports: [CommonModule, IconModule, MessageDisplayModule, SkeletonModule],
exports: [LoadAsyncDirective]
})
export class LoadAsyncModule {}
9 changes: 8 additions & 1 deletion projects/components/src/load-async/load-async.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,14 @@ export type AsyncState = LoadingAsyncState | SuccessAsyncState | NoDataOrErrorAs
export const enum LoaderType {
Spinner = 'spinner',
ExpandableRow = 'expandable-row',
Page = 'page'
Page = 'page',
Rectangle = 'rectangle',
Text = 'text',
Square = 'square',
Circle = 'circle',
TableRow = 'table-row',
ListItem = 'list-item',
Donut = 'donut'
}

interface LoadingAsyncState {
Expand Down
11 changes: 7 additions & 4 deletions projects/components/src/load-async/loader/loader.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,6 @@
.ht-loader {
width: 100%;
height: 100%;
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;

.page {
height: 50px;
Expand All @@ -23,3 +19,10 @@
width: auto;
}
}

.flex-centered {
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
}
91 changes: 91 additions & 0 deletions projects/components/src/load-async/loader/loader.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { CommonModule } from '@angular/common';
import { ImagesAssetPath } from '@hypertrace/assets-library';
import { createHostFactory, SpectatorHost } from '@ngneat/spectator/jest';
import { MockComponent } from 'ng-mocks';
import { SkeletonComponent, SkeletonType } from '../../skeleton/skeleton.component';
import { LoaderType } from '../load-async.service';
import { LoaderComponent } from './loader.component';

Expand All @@ -9,13 +11,15 @@ describe('Loader component', () => {

const createHost = createHostFactory({
component: LoaderComponent,
declarations: [MockComponent(SkeletonComponent)],
imports: [CommonModule]
});

test('Loader component when loader type is page', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.Page}'"></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).toHaveClass('flex-centered');
expect(spectator.query('.ht-loader img')).toExist();
expect(spectator.query('.ht-loader img')).toHaveClass(LoaderType.Page);
expect(spectator.query('.ht-loader img')).toHaveAttribute('src', ImagesAssetPath.LoaderPage);
Expand All @@ -25,6 +29,7 @@ describe('Loader component', () => {
spectator = createHost(`<ht-loader></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).toHaveClass('flex-centered');
expect(spectator.query('.ht-loader img')).toExist();
expect(spectator.query('.ht-loader img')).toHaveClass(LoaderType.Spinner);
expect(spectator.query('.ht-loader img')).toHaveAttribute('src', ImagesAssetPath.LoaderSpinner);
Expand All @@ -34,6 +39,7 @@ describe('Loader component', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.Spinner}'"></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).toHaveClass('flex-centered');
expect(spectator.query('.ht-loader img')).toExist();
expect(spectator.query('.ht-loader img')).toHaveClass(LoaderType.Spinner);
expect(spectator.query('.ht-loader img')).toHaveAttribute('src', ImagesAssetPath.LoaderSpinner);
Expand All @@ -43,8 +49,93 @@ describe('Loader component', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.ExpandableRow}'"></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).toHaveClass('flex-centered');
expect(spectator.query('.ht-loader img')).toExist();
expect(spectator.query('.ht-loader img')).toHaveClass(LoaderType.ExpandableRow);
expect(spectator.query('.ht-loader img')).toHaveAttribute('src', ImagesAssetPath.LoaderExpandableRow);
});

test('Should use old loader type by default', () => {
spectator = createHost(`<ht-loader></ht-loader>`);

expect(spectator.component.isOldLoaderType).toBe(true);
expect(spectator.query(SkeletonComponent)).not.toExist();
});

test('Should use corresponding skeleton component for loader type rectangle', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.Rectangle}'" ></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).not.toHaveClass('flex-centered');

const skeletonComponent = spectator.query(SkeletonComponent);
expect(skeletonComponent).toExist();
expect(skeletonComponent).toHaveAttribute('skeletonType', SkeletonType.Rectangle);
});

test('Should use corresponding skeleton component for loader type rectangle text', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.Text}'" ></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).not.toHaveClass('flex-centered');

const skeletonComponent = spectator.query(SkeletonComponent);
expect(skeletonComponent).toExist();
expect(skeletonComponent).toHaveAttribute('skeletonType', SkeletonType.Text);
});

test('Should use corresponding skeleton component for loader type circle', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.Circle}'" ></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).not.toHaveClass('flex-centered');

const skeletonComponent = spectator.query(SkeletonComponent);
expect(skeletonComponent).toExist();
expect(skeletonComponent).toHaveAttribute('skeletonType', SkeletonType.Circle);
});

test('Should use corresponding skeleton component for loader type square', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.Square}'" ></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).not.toHaveClass('flex-centered');

const skeletonComponent = spectator.query(SkeletonComponent);
expect(skeletonComponent).toExist();
expect(skeletonComponent).toHaveAttribute('skeletonType', SkeletonType.Square);
});

test('Should use corresponding skeleton component for loader type table row', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.TableRow}'" ></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).not.toHaveClass('flex-centered');

const skeletonComponent = spectator.query(SkeletonComponent);
expect(skeletonComponent).toExist();
expect(skeletonComponent).toHaveAttribute('skeletonType', SkeletonType.TableRow);
});

test('Should use corresponding skeleton component for loader type donut', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.Donut}'" ></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).not.toHaveClass('flex-centered');

const skeletonComponent = spectator.query(SkeletonComponent);
expect(skeletonComponent).toExist();
expect(skeletonComponent).toHaveAttribute('skeletonType', SkeletonType.Donut);
});

test('Should use corresponding skeleton component for loader type list item', () => {
spectator = createHost(`<ht-loader [loaderType]="'${LoaderType.ListItem}'" ></ht-loader>`);

expect(spectator.query('.ht-loader')).toExist();
expect(spectator.query('.ht-loader')).not.toHaveClass('flex-centered');

const skeletonComponent = spectator.query(SkeletonComponent);
expect(skeletonComponent).toExist();
expect(skeletonComponent).toHaveAttribute('skeletonType', SkeletonType.ListItem);
});
});
64 changes: 62 additions & 2 deletions projects/components/src/load-async/loader/loader.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,66 @@
import { ChangeDetectionStrategy, Component, Input, OnChanges } from '@angular/core';
import { ImagesAssetPath } from '@hypertrace/assets-library';
import { assertUnreachable } from '@hypertrace/common';
import { SkeletonType } from '../../skeleton/skeleton.component';
import { LoaderType } from '../load-async.service';

@Component({
selector: 'ht-loader',
styleUrls: ['./loader.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div class="ht-loader">
<img [ngClass]="[this.currentLoaderType]" [src]="this.getImagePathFromType(this.currentLoaderType)" />
<div class="ht-loader" [ngClass]="{ 'flex-centered': this.isOldLoaderType }">
<ng-container *ngIf="!this.isOldLoaderType; else oldLoaderTemplate">
<ht-skeleton [skeletonType]="this.skeletonType"></ht-skeleton>
</ng-container>

<ng-template #oldLoaderTemplate>
<img [ngClass]="[this.currentLoaderType]" [src]="this.imagePath" />
</ng-template>
</div>
`
})
export class LoaderComponent implements OnChanges {
@Input()
public loaderType?: LoaderType;

public skeletonType: SkeletonType = SkeletonType.Rectangle;

public currentLoaderType: LoaderType = LoaderType.Spinner;

public imagePath: ImagesAssetPath = ImagesAssetPath.LoaderSpinner;

public isOldLoaderType: boolean = true;

public ngOnChanges(): void {
this.currentLoaderType = this.loaderType ?? LoaderType.Spinner;

if (this.determineIfOldLoaderType(this.currentLoaderType)) {
this.isOldLoaderType = true;
this.imagePath = this.getImagePathFromType(this.currentLoaderType);
} else {
this.isOldLoaderType = false;
this.skeletonType = this.getSkeletonTypeForLoader(this.currentLoaderType);
}
}

public determineIfOldLoaderType(loaderType: LoaderType): boolean {
switch (loaderType) {
case LoaderType.Spinner:
case LoaderType.ExpandableRow:
case LoaderType.Page:
return true;
case LoaderType.Circle:
case LoaderType.Text:
case LoaderType.ListItem:
case LoaderType.Rectangle:
case LoaderType.Square:
case LoaderType.TableRow:
case LoaderType.Donut:
return false;
default:
return assertUnreachable(loaderType);
}
}

public getImagePathFromType(loaderType: LoaderType): ImagesAssetPath {
Expand All @@ -33,4 +74,23 @@ export class LoaderComponent implements OnChanges {
return ImagesAssetPath.LoaderSpinner;
}
}

public getSkeletonTypeForLoader(curLoaderType: LoaderType): SkeletonType {
switch (curLoaderType) {
case LoaderType.Text:
return SkeletonType.Text;
case LoaderType.Circle:
return SkeletonType.Circle;
case LoaderType.Square:
return SkeletonType.Square;
case LoaderType.TableRow:
return SkeletonType.TableRow;
case LoaderType.ListItem:
return SkeletonType.ListItem;
case LoaderType.Donut:
return SkeletonType.Donut;
default:
return SkeletonType.Rectangle;
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 expect to hit this, so assertUnreachable would be a good fit 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.

wouldn't we hit it if the input loader type is either ExpandableRow, Page, or Spinner ? I attempted adding it but it wouldn't let me pass in curLoaderType :

Argument of type 'import("/Users/christian/traceable-ui/hypertrace-ui/projects/components/src/load-async/load-async.service").LoaderType' is not assignable to parameter of type 'never'

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that not every case was listed here, but that's actually the point of assertUnreachable - it will give that compiler error you saw if a case isn't handled. So what I'd suggest is changing

      default:
        return SkeletonType.Rectangle;

to

      case LoaderType.ExpandableRow:
      case LoaderType.Page:
      case LoaderType.Spinner:
        return SkeletonType.Rectangle;  
      default:
        return assertUnreachable(curLoaderType);

Although the behavior is currently identical, when a new loader type is added we're forcing the author to come look at this switch statement and make a decision on how to map it, rather than them potentially incorrectly being mapped to the default case.

Copy link
Contributor

Choose a reason for hiding this comment

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

case LoaderType.ExpandableRow: 
case LoaderType.Page: 
case LoaderType.Spinner: 
return SkeletonType.Rectangle;

These cases wouldn't be used anyway since we only show skeleton component when we see the new "loader type".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, as Anand said, we only end up using the return type from this switch if the loader type is not one of the three older types, so including them here is irrelevant, except for purpose of using the assertUnreachable. And with recent changes coming the switch statement won't even be hit if loader type is one of the three old types. So i'm going to revert it back to the original version using the default case.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's trading a compiler check for a logic check - never a good trade. If you want the best of both worlds, the types can actually be improved to represent the behavior you want - make the loader type a union of two enums, one representing the old values, one the new. This function would only accept the new ones (or you may not even need to map types, since the same skeleton type enum can be used by both)

Copy link
Contributor

@anandtiwary anandtiwary Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, we could do that. But In a follow-up PR, we plan to migrate existing usages to new skeleton loaders, and then we will remove all the three older loader types and associated code. If you prefer, we can bring these cases back for correctness in the meantime.

case LoaderType.ExpandableRow: 
case LoaderType.Page: 
case LoaderType.Spinner: 
return SkeletonType.Rectangle;

Copy link
Contributor

Choose a reason for hiding this comment

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

If they're going to go away in a followup PR, the style isn't a big deal since that's just for maintainability. In that case either way is fine with me as long as the logic is right.

}
}
}
Loading