Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
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
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,13 @@ export type AsyncState = LoadingAsyncState | SuccessAsyncState | NoDataOrErrorAs
export const enum LoaderType {
Spinner = 'spinner',
ExpandableRow = 'expandable-row',
Page = 'page'
Page = 'page',
Rectangle = 'rectangle',
RectangleText = 'rectangle-text',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename it to 'Text'?

Square = 'square',
Circle = 'circle',
TableRow = 'table-row',
ListItem = 'list-item'
}

interface LoadingAsyncState {
Expand All @@ -80,6 +86,7 @@ interface NoDataOrErrorAsyncState {

interface LoadingStateConfig {
loaderType?: LoaderType;
repeatCount?: number;
}

interface NoDataOrErrorStateConfig {
Expand Down
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;
}
43 changes: 41 additions & 2 deletions projects/components/src/load-async/loader/loader.component.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,39 @@
import { ChangeDetectionStrategy, Component, Input, OnChanges } from '@angular/core';
import { ImagesAssetPath } from '@hypertrace/assets-library';
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.oldLoaderType() }">
<ng-container *ngIf="!this.oldLoaderType(); else oldLoader">
<ht-skeleton [shapeStyle]="this.skeletonType" [repeat]="this.repeatLoaderCount"></ht-skeleton>
</ng-container>

<ng-template #oldLoader>
Copy link
Contributor

Choose a reason for hiding this comment

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

template names should be suffixed with template

<img [ngClass]="[this.currentLoaderType]" [src]="this.getImagePathFromType(this.currentLoaderType)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

since the image path is dependent on currentLoaderType, this can also be a property computed in ngOnChanges. Alternatively, use htMemoize here

</ng-template>
</div>
`
})
export class LoaderComponent implements OnChanges {
@Input()
public loaderType?: LoaderType;

@Input()
public repeatLoaderCount?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: name can be shortened to repeatCount


public skeletonType: SkeletonType = SkeletonType.Rectangle;

public currentLoaderType: LoaderType = LoaderType.Spinner;

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

this.skeletonType = this.loaderToSkeletonTypeMap(this.currentLoaderType);
}

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

public loaderToSkeletonTypeMap(curLoaderType: LoaderType): SkeletonType {
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't really a map. It is a function. getSkeletonTypeForLoader is a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

that suggestion works for me, but map is really the verb here so mapLoaderToSkeletonType would also be fine.

switch (curLoaderType) {
case LoaderType.RectangleText:
return SkeletonType.RectangleText;
case LoaderType.Circle:
return SkeletonType.Circle;
case LoaderType.Square:
return SkeletonType.Square;
case LoaderType.TableRow:
return SkeletonType.TableRow;
case LoaderType.ListItem:
return SkeletonType.ListItem;
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

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.

}
}

public oldLoaderType(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

better name for a boolean function is isOldLoaderType

return (
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 a method for this. You can make this a property in the class and assign value to it on changes, after L34. More efficient.

this.currentLoaderType === LoaderType.ExpandableRow ||
this.currentLoaderType === LoaderType.Spinner ||
this.currentLoaderType === LoaderType.Page
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const ASYNC_WRAPPER_PARAMETERS$ = new InjectionToken<Observable<LoadAsync
template: `
<div *ngIf="this.state$ | async as state" class="fill-container" [ngSwitch]="state.type">
<ng-container *ngSwitchCase="'${LoadAsyncStateType.Loading}'">
<ht-loader [loaderType]="this.loaderType"></ht-loader>
<ht-loader [loaderType]="this.loaderType" [repeatLoaderCount]="this.config?.load?.repeatCount"></ht-loader>
</ng-container>
<ng-container *ngSwitchCase="'${LoadAsyncStateType.Success}'">
<ng-container *ngTemplateOutlet="this.content; context: state.context"></ng-container>
Expand Down
114 changes: 114 additions & 0 deletions projects/components/src/skeleton/skeleton.component.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
@mixin loading-animation {
content: '';
animation: skeleton-animation 1.2s infinite;
height: 100%;
left: 0;
position: absolute;
right: 0;
top: 0;
transform: translateX(-100%);
z-index: 1;
background: linear-gradient(90deg, rgba(255, 255, 255, 0), rgba(255, 255, 255, 0.4), rgba(255, 255, 255, 0));
}

@mixin initial-location {
top: initial;
left: initial;
transform: initial;
}

.skeleton {
position: relative;
background-color: #dee2e6;
Copy link
Contributor

Choose a reason for hiding this comment

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

use a color value from _color-palette.scss. If the required color is not defined, either use a suitable closest approximation or add an appropriate value there and consume it here. No explicit hex usages.

overflow: hidden;

top: 50%;
left: 50%;
transform: translate(-50%, -50%);

border-radius: 6px;
}

.skeleton::after {
@include loading-animation;
}

.skeleton.skeleton-rectangle {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of enumerating all child classes this way, you can do the following

.skeleton {
  &.skeleton-rectangle {}
  &.skeleton-rectangle-text {}
  ...
}

height: 80%;
width: 80%;
}

.skeleton.skeleton-rectangle-text {
height: 1.3rem;
width: 90%;
}

.skeleton.skeleton-circle {
width: 2rem;
height: 2rem;
border-radius: 50%;
}

.skeleton.skeleton-square {
width: 3rem;
height: 3rem;
}

.skeleton.skeleton-table-row {
height: 1.3rem;
width: 90%;

@include initial-location;
Copy link
Contributor

Choose a reason for hiding this comment

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

inclusions should be in the beginning of a css rule definition so that overrides are clear.

}

.skeleton.skeleton-repeating {
margin-top: 1rem;
}

.skeleton.skeleton-list-item {
background-color: rgb(255, 255, 255);
Copy link
Contributor

Choose a reason for hiding this comment

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

use a color constant.

@include initial-location;
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here. inclusions in the beginning.

display: flex;
justify-content: flex-start;

.item-circle {
background-color: #dee2e6;
Copy link
Contributor

Choose a reason for hiding this comment

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

no explicit color hex codes.

height: 2rem;
width: 2rem;
border-radius: 50%;
}
.item-circle::after {
@include loading-animation;
}

.item-column {
display: flex;
flex-direction: column;
padding-left: 1rem;
width: 100%;
}

.item-column > div {
Copy link
Contributor

Choose a reason for hiding this comment

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

do not use element styling. apply a class to the children and style the class.

background-color: #dee2e6;
Copy link
Contributor

Choose a reason for hiding this comment

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

no explicit color hex codes.

height: 0.9rem;
border-radius: 6px;
width: 90%;

&:last-child {
margin-top: 5px;
width: 80%;
}
}
.item-column > div::after {
@include loading-animation;
}
}

@keyframes skeleton-animation {
from {
transform: translateX(-100%);
}
to {
transform: translateX(100%);
}
}
65 changes: 65 additions & 0 deletions projects/components/src/skeleton/skeleton.component.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { ChangeDetectionStrategy, Component, Input } from '@angular/core';

interface ContainerClass {
skeleton: boolean;
'skeleton-rectangle': boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

use camelCasing for the property keys. This will also help you avoid string usages in L56-62.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These property names are used as class names, does that still apply ? camel case class names ?

'skeleton-square': boolean;
'skeleton-circle': boolean;
'skeleton-rectangle-text': boolean;
'skeleton-table-row': boolean;
'skeleton-list-item': boolean;
'skeleton-repeating': boolean;
}

export const enum SkeletonType {
Rectangle = 'rectangle',
RectangleText = 'rectangle-text',
Square = 'square',
Circle = 'circle',
TableRow = 'table-row',
ListItem = 'list-item'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

move the types to the bottom of the file, after component definition


@Component({
selector: 'ht-skeleton',
template: `
<ng-container *ngFor="let k of [].constructor(this.repeat)">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this array construction into TS for readability (ideally just do something like this.repeat && new Array(this.repeat) Also, be careful with undefined values here.

<ng-container [ngSwitch]="this.shapeStyle">
<div *ngSwitchCase="'${SkeletonType.RectangleText}'" [ngClass]="containerClass()"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Careful with function calls, especially ones that return new objects like this. Instead, just calculate the class dictionary in ngOnChanges.

<div *ngSwitchCase="'${SkeletonType.Circle}'" [ngClass]="containerClass()"></div>
<div *ngSwitchCase="'${SkeletonType.Square}'" [ngClass]="containerClass()"></div>
<div *ngSwitchCase="'${SkeletonType.TableRow}'" [ngClass]="containerClass()"></div>
<div *ngSwitchCase="'${SkeletonType.ListItem}'" [ngClass]="containerClass()">
<div class="item-circle"></div>
<div class="item-column">
<div class="line-one"></div>
<div class="line-two"></div>
</div>
</div>
<div *ngSwitchDefault class="skeleton skeleton-rectangle"></div>
</ng-container>
</ng-container>
`,
changeDetection: ChangeDetectionStrategy.OnPush,
styleUrls: ['./skeleton.component.scss']
})
export class SkeletonComponent {
@Input() public shapeStyle: SkeletonType = 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.

please follow the style guidelines from existing components.
Line break between annotation and the declaration.


@Input() public repeat: number = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

In loader we're assigning an optional field to this input, make sure to handle undefined and fix types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate on 'fix types' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

we're assigning something of type number | undefined, so we should declare that here - that'll force the compiler to keep us honest.

Suggested change
@Input() public repeat: number = 1;
@Input()
public repeat: number | undefined = 1;


@Input() public size: string = '';

public containerClass(): ContainerClass {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to getContainerClass

return {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a property, updated when shapeStyle or repeat values are changed.

skeleton: true,
'skeleton-rectangle': this.shapeStyle === 'rectangle',
Copy link
Contributor

Choose a reason for hiding this comment

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

use the enum on the RHS.

'skeleton-square': this.shapeStyle === 'square',
'skeleton-circle': this.shapeStyle === 'circle',
'skeleton-rectangle-text': this.shapeStyle === 'rectangle-text',
'skeleton-table-row': this.shapeStyle === 'table-row',
'skeleton-list-item': this.shapeStyle === 'list-item',
'skeleton-repeating': this.repeat > 1
};
}
}
10 changes: 10 additions & 0 deletions projects/components/src/skeleton/skeleton.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { SkeletonComponent } from './skeleton.component';

@NgModule({
declarations: [SkeletonComponent],
imports: [CommonModule],
exports: [SkeletonComponent]
})
export class SkeletonModule {}