feat: Skeleton loader for LoadAsync directive

[Christian862]

Description

To finish the work of the LoadAsync directive, the SkeletonComponent is used to provide loading animations. It has the option of a few different shapes. Specify how many times to repeat the skeleton component by providing the repeatCount to the LoadAsyncConfig input - Note, as of this v1, repeatCount should only be used with LoaderType.RectangleText, LoaderType.TableRow and LoaderType.ListItem.

Testing

Updated LoaderComponent test to cover each possible new LoaderType and testing to ensure old LoaderType is used by default. Added testing for SkeletonComponent, testing the expected default behaviour, testing the repeat input behaviour and testing the correct display for the respective SkeletonType. For the last test in SkeletonComponent, 'Should match the skeleton type to the corresponding element', i'm not sure if this is the best approach, thoughts ?

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

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

[codecov]

Codecov Report

Merging #1373 (0b7c6b4) into main (f9af505) will increase coverage by 0.05%.
The diff coverage is 97.18%.

@@            Coverage Diff             @@
##             main    #1373      +/-   ##
==========================================
+ Coverage   84.48%   84.53%   +0.05%     
==========================================
  Files         841      843       +2     
  Lines       17861    17932      +71     
  Branches     2333     2357      +24     
==========================================
+ Hits        15090    15159      +69     
- Misses       2660     2662       +2     
  Partials      111      111              

Impacted Files	Coverage Δ
...ts/components/src/load-async/load-async.service.ts	100.00% <ø> (ø)
...mponents/src/load-async/loader/loader.component.ts	96.49% <95.23%> (-3.51%)	⬇️
...cts/components/src/load-async/load-async.module.ts	100.00% <100.00%> (ø)
...ects/components/src/skeleton/skeleton.component.ts	100.00% <100.00%> (ø)
...rojects/components/src/skeleton/skeleton.module.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 f9af505...0b7c6b4. Read the comment docs.

[arjunlalb]

better name for a boolean function is isOldLoaderType

[arjunlalb]

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

[aaron-steinfeld]

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

[arjunlalb]

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.

[arjunlalb]

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

[arjunlalb]

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

[arjunlalb]

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

[arjunlalb]

no explicit color hex codes.

[arjunlalb]

no explicit color hex codes.

[arjunlalb]

use a color constant.

[arjunlalb]

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

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

[aaron-steinfeld]

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

[aaron-steinfeld]

We don't expect to hit this, so assertUnreachable would be a good fit here.

[Christian862]

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'

[aaron-steinfeld]

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.

[anandtiwary]

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

[Christian862]

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.

[aaron-steinfeld]

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)

[anandtiwary]

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;

[aaron-steinfeld]

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.

[aaron-steinfeld]

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.

[aaron-steinfeld]

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

[aaron-steinfeld]

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

[Christian862]

Could you elaborate on 'fix types' ?

[aaron-steinfeld]

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;

[anandtiwary]

nit: can we call it skeletonType?

[anandtiwary]

Can we declare skeleton and repeating as static string properties in the component?

[anandtiwary]

private static readonly SKELETON_CLASS_NAME = 'skeleton';
private static readonly REPEATING_CLASS_NAME = 'repeating';

[anandtiwary]

nit: we usually keep static properties at the top, right after class definition.

[github-actions]

Unit Test Results

       4 files  ±  0     277 suites  +1   23m 3s ⏱️ + 2m 7s
   997 tests +11     997 ✔️ +11  0 💤 ±0  0 ❌ ±0 
1 005 runs  +11  1 005 ✔️ +11  0 💤 ±0  0 ❌ ±0 

Results for commit 150d5eb. ± Comparison against base commit f9af505.