Initial draft for new loaders using gifs #1071

alok-traceable · 2021-08-17T06:35:00Z

Description

New gif loader.

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

Usages

As directive

*htLoadAsync="applicationInsights.apiDistributionByRiskSeries$ as apiDistributionByRiskSeries; loaderType: '${LoaderTypes.LargeSquare}'"

As component

<ht-loader [type]="${LoaderTypes.LargeSquare}"></ht-loader>

codecov · 2021-08-17T06:39:15Z

Codecov Report

Merging #1071 (76d5ffd) into main (594a8db) will increase coverage by 0.00%.
The diff coverage is 91.30%.

@@           Coverage Diff           @@
##             main    #1071   +/-   ##
=======================================
  Coverage   85.08%   85.09%           
=======================================
  Files         825      825           
  Lines       17066    17086   +20     
  Branches     2216     2220    +4     
=======================================
+ Hits        14521    14539   +18     
- Misses       2514     2516    +2     
  Partials       31       31

Impacted Files	Coverage Δ
...ts/components/src/not-found/not-found.component.ts	`100.00% <ø> (ø)`
...mponents/src/load-async/loader/loader.component.ts	`88.23% <88.23%> (-11.77%)`	⬇️
projects/assets-library/src/public-api.ts	`100.00% <100.00%> (ø)`
.../components/src/load-async/load-async.directive.ts	`100.00% <100.00%> (ø)`
...load-async/wrapper/load-async-wrapper.component.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 594a8db...76d5ffd. Read the comment docs.

anandtiwary · 2021-08-17T18:00:35Z

Discussed this in our call today. Overall a good change.

Few Action items:

Rename size to type
Rename Large, Medium, Small to better names like Page, widget etc. We can look into how UX team is planning to use them and then decide the appropriate name
Add a similar type property to htLoadAsync directive and propagate it to the loader wrapper and the loader component. Set an appropriate default value that would take care of majority of cases.
Then go and set specific value to any loader which needs a different loader than the default one.

alok-traceable · 2021-08-19T08:10:36Z

Discussed this in our call today. Overall a good change.

Few Action items:

Rename size to type

Rename Large, Medium, Small to better names like Page, widget etc. We can look into how UX team is planning to use them and then decide the appropriate name

Add a similar type property to htLoadAsync directive and propagate it to the loader wrapper and the loader component. Set an appropriate default value that would take care of majority of cases.

Then go and set specific value to any loader which needs a different loader than the default one.

updated, please check the description I have added usage section

projects/assets-library/src/images/image-type.ts

anandtiwary · 2021-08-19T18:32:18Z

projects/components/src/load-async/load-async.directive.ts

+    // 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({


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.

anandtiwary · 2021-08-19T18:33:00Z

projects/assets-library/src/images/image-type.ts

+  LoaderHorizontal = 'assets/images/loader-horizontal.gif'
+}
+
+export const enum LoaderTypes {


anandtiwary · 2021-08-19T20:27:25Z

projects/assets-library/src/images/image-type.ts

+export const enum LoaderTypes {
+  SmallCircle = 'small-circle',
+  Horizontal = 'horizontal',
+  LargeSquare = 'large-square'


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'.

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

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.

projects/components/src/load-async/loader/loader.component.ts

anandtiwary · 2021-08-19T20:31:11Z

projects/components/src/load-async/loader/loader.component.ts

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


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

alok-traceable · 2021-08-31T05:23:04Z

I will continue on writing test cases, is there anything else pending on this PR?
ping: @anandtiwary

anandtiwary · 2021-08-31T18:41:49Z

projects/components/src/load-async/load-async.directive.ts

@@ -23,8 +24,11 @@ import {
 export class LoadAsyncDirective implements OnChanges, OnDestroy {
  @Input('htLoadAsync')
  public data$?: Observable<unknown>;
+  @Input()
+  public htLoadAsyncLoaderType!: LoaderType;


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

anandtiwary · 2021-08-31T18:42:22Z

projects/components/src/load-async/load-async.directive.ts

  private readonly wrapperParamsSubject: ReplaySubject<LoadAsyncWrapperParameters> = new ReplaySubject(1);
-  private readonly wrapperInjector: Injector;
+  private wrapperInjector!: Injector;


any reason for removing the readonly?

anandtiwary · 2021-08-31T18:43:26Z

projects/components/src/load-async/loader/loader.component.ts

+export class LoaderComponent implements OnChanges {
+  @Input()
+  public type: LoaderType = LoaderType.ExpandableRow;
+  public imagePath: ImagesAssetPath = ImagesAssetPath.LoaderExpandableRow;


Is formatting right? Can add a new line after L17

anandtiwary · 2021-08-31T18:44:19Z

projects/components/src/load-async/loader/loader.component.ts

-export class LoaderComponent {}
+export class LoaderComponent implements OnChanges {
+  @Input()
+  public type: LoaderType = LoaderType.ExpandableRow;


Should this be the default?

anandtiwary · 2021-08-31T18:46:53Z

projects/components/src/load-async/wrapper/load-async-wrapper.component.ts

@@ -39,9 +40,13 @@ export class LoadAsyncWrapperComponent {

  public content?: TemplateRef<LoadAsyncContext>;

+  private readonly loaderTypeSubject: BehaviorSubject<LoaderType> = new BehaviorSubject<LoaderType>(LoaderType.Spinner);


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

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

anandtiwary · 2021-09-07T15:08:04Z

projects/components/src/load-async/load-async.directive.ts

  private readonly wrapperParamsSubject: ReplaySubject<LoadAsyncWrapperParameters> = new ReplaySubject(1);
-  private readonly wrapperInjector: Injector;
+  private readonly wrapperInjector!: Injector;


Is this required?

anandtiwary · 2021-09-07T15:10:56Z

projects/components/src/load-async/wrapper/load-async-wrapper.component.ts

 @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>


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

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

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

anandtiwary · 2021-09-07T15:16:45Z

projects/components/src/load-async/wrapper/load-async-wrapper.component.ts

  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)),


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

anandtiwary · 2021-09-07T15:17:39Z

projects/components/src/load-async/loader/loader.component.ts

-export class LoaderComponent {}
+export class LoaderComponent implements OnChanges {
+  @Input()
+  public type!: LoaderType;


This property can be optional right?

anandtiwary · 2021-09-07T15:23:16Z

projects/components/src/load-async/loader/loader.component.ts

+      case LoaderType.Spinner:
+        return ImagesAssetPath.LoaderSpinner;
+      default:
+        return assertUnreachable(this.type);


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

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.

anandtiwary · 2021-09-21T03:54:37Z

projects/components/src/load-async/loader/loader.component.test.ts

+import { createHostFactory, SpectatorHost } from '@ngneat/spectator/jest';
+import { LoaderComponent } from './loader.component';
+
+describe('Link component', () => {


anandtiwary · 2021-09-21T03:55:52Z

projects/components/src/load-async/load-async.directive.ts

@@ -23,8 +24,11 @@ import {
 export class LoadAsyncDirective implements OnChanges, OnDestroy {
  @Input('htLoadAsync')
  public data$?: Observable<unknown>;
+  @Input()


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

This should work

anandtiwary · 2021-09-21T04:02:59Z

projects/components/src/load-async/load-async.directive.ts

@@ -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,


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

anandtiwary · 2021-09-21T04:04:46Z

projects/components/src/load-async/loader/loader.component.ts

+  @Input()
+  public type: LoaderType = LoaderType.Spinner;
+
+  public imagePath: ImagesAssetPath = ImagesAssetPath.LoaderSpinner;


If we keep it unassigned, would it get the same value from getImagePathFromType call inside ngOnChanges?

anandtiwary · 2021-09-21T04:08:44Z

projects/components/src/load-async/wrapper/load-async-wrapper.component.ts

  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)),
      switchMap(parameter => parameter.state$),


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.

github-actions · 2021-09-22T07:47:07Z

Unit Test Results

    4 files ±0 268 suites +1 17m 31s ⏱️ +57s
966 tests +4 966 ✔️ +4 0 💤 ±0 0 ❌ ±0
973 runs +4 973 ✔️ +4 0 💤 ±0 0 ❌ ±0

Results for commit 76d5ffd. ± Comparison against base commit 594a8db.

anandtiwary · 2021-09-22T15:31:11Z

@alok-traceable I am guessing this is not ready for another round of review, right?

anandtiwary · 2021-10-29T03:47:51Z

@itssharmasandeep Can we close this one?

itssharmasandeep · 2021-10-29T06:42:00Z

Closing this, as #1217 contains the same changes from this one

fix: initial draft for new loaders

7d84a9d

alok-traceable requested a review from a team as a code owner August 17, 2021 06:35

This comment has been minimized.

Sign in to view

fix: lint fixes

5b1e115

This comment has been minimized.

Sign in to view

alok-traceable added 3 commits August 19, 2021 11:59

fix: pr suggestions

9569dfa

Merge branch 'main' into new-loader-component

3ef430d

fix: pr suggestions

752b534

This comment has been minimized.

Sign in to view

alok-traceable added 2 commits August 19, 2021 16:24

fix: pr suggestions

579ae96

fix: pr suggestions

ede6f6e

This comment has been minimized.

Sign in to view

anandtiwary reviewed Aug 19, 2021

View reviewed changes

projects/assets-library/src/images/image-type.ts Show resolved Hide resolved

anandtiwary reviewed Aug 19, 2021

View reviewed changes

projects/components/src/load-async/loader/loader.component.ts Show resolved Hide resolved

anandtiwary reviewed Aug 19, 2021

View reviewed changes

alok-traceable added 2 commits August 27, 2021 13:17

Merge branch 'main' into new-loader-component

ff67f22

fix: pr suggestions

aa03ccb

This comment has been minimized.

Sign in to view

anandtiwary reviewed Aug 31, 2021

View reviewed changes

alok-traceable added 2 commits September 6, 2021 20:51

Merge branch 'main' into new-loader-component

b34414a

fix: adding test cases and pr suggestions

b7444b2

This comment has been minimized.

Sign in to view

anandtiwary reviewed Sep 7, 2021

View reviewed changes

Merge branch 'main' into new-loader-component

0470ca4

This comment has been minimized.

Sign in to view

alok-traceable and others added 2 commits September 9, 2021 11:08

fix: adding test cases and default values

d51d783

Merge branch 'main' into new-loader-component

c78b305

This comment has been minimized.

Sign in to view

anandtiwary reviewed Sep 21, 2021

View reviewed changes

alok-traceable added 2 commits September 22, 2021 11:50

Merge branch 'main' into new-loader-component

416837b

fix: pr suggestions

76d5ffd

itssharmasandeep closed this Oct 29, 2021

		@@ -39,9 +40,13 @@ export class LoadAsyncWrapperComponent {

		public content?: TemplateRef<LoadAsyncContext>;

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

Initial draft for new loaders using gifs #1071

Initial draft for new loaders using gifs #1071

Uh oh!

Conversation

alok-traceable commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Usages

As directive

As component

Uh oh!

codecov bot commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

This comment has been minimized.

anandtiwary commented Aug 17, 2021

Uh oh!

This comment has been minimized.

alok-traceable commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

alok-traceable commented Aug 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

alok-traceable commented Aug 17, 2021 •

edited

Loading

codecov bot commented Aug 17, 2021 •

edited

Loading

alok-traceable commented Aug 19, 2021 •

edited

Loading

anandtiwary commented Sep 22, 2021 •

edited

Loading