feat: custom messages for load async

[itssharmasandeep]

Description

We can have custom message configs for different states for load async warpper.
Here is one example for error state

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

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 #1217 (35c7d50) into main (981e14b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1217      +/-   ##
==========================================
+ Coverage   84.69%   84.72%   +0.02%     
==========================================
  Files         832      832              
  Lines       17333    17362      +29     
  Branches     2260     2272      +12     
==========================================
+ Hits        14681    14710      +29     
  Misses       2618     2618              
  Partials       34       34              

Impacted Files	Coverage Δ
...ts/components/src/load-async/load-async.service.ts	100.00% <ø> (ø)
...ts/components/src/not-found/not-found.component.ts	100.00% <ø> (ø)
...d/widgets/table/table-widget-renderer.component.ts	42.72% <ø> (ø)
projects/assets-library/src/public-api.ts	100.00% <100.00%> (ø)
.../components/src/load-async/load-async.directive.ts	100.00% <100.00%> (ø)
...mponents/src/load-async/loader/loader.component.ts	100.00% <100.00%> (ø)
...load-async/wrapper/load-async-wrapper.component.ts	100.00% <100.00%> (ø)
projects/components/src/public-api.ts	100.00% <100.00%> (ø)
projects/components/src/table/table.component.ts	89.74% <100.00%> (+0.03%)	⬆️
...dashboard/widgets/table/table-widget-base.model.ts	76.19% <100.00%> (+0.58%)	⬆️

---

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 981e14b...35c7d50. Read the comment docs.

[anandtiwary]

This won't react to changes in "config" object. Check L52 and also no CD will be triggered if object property is modified.

[anandtiwary]

Also, if we update or emit again on the state$, the entire content would be re-rendered. There is no benefit in doing that.

So my recommendation would be to expose these properties directly under this directive.

1. loaderType
2. noData message

Then you can just add it to the wrapperParamsSubject (instead of state$)

For error messages, we had decided to only show "internal error". (Not sure if anything has changed on this, but lemme know)

[anandtiwary]

The two properties would then be used inside the loader-wrapper.component. No changes should be needed in the service.

Since state$ is not modified by config change, the wrapper component would not re-render the contents.

However, you can possibly map the parameters observable and extract loaderType$ and noDataMessage$ and use it with async pipe directly in the template. This would pass on the "change" immediately.

[itssharmasandeep]

I thought of doing this in a generic way, because if in future, we need new error messages, we would end up here.
If this is what we agree on, I can make the changes tomorrow.

extract loaderType$ and noDataMessage$ and use it with async pipe directly in the template.

I don't think we would require to do this. We can directly use the property and pass, why we would need an observable of loaderType and use of async pipe.

This won't react to changes in "config" object. Check L52 and also no CD will be triggered if object property is modified.

The other option would be, I can directly pass these configs to wrapperParamsSubject and use it in the loader-wrapper.component, in that way it would be generic enough and there will no issues like mentioned above

[anandtiwary]

I don't think we would require to do this. We can directly use the property as pass, why we would need an observable of loaderType and use of async pipe.

Yeah, you can use tap too. But the subscription is happening via state$, so just ensure we don't end up emitting again on state$.

[anandtiwary]

The other option would be, I can directly pass these configs to wrapperParamsSubject and use it in the loader-wrapper.component, in that way it would be generic enough and there will no issues like mentioned above

You can, but this won't detect any property change in the config. You will have to create a new config object each time you change any property.

[aaron-steinfeld]

This won't react to changes in "config" object.

Yes it will, we're inside ngOnChanges (the condition checks if there is a data input, not wether it changed)

Also, if we update or emit again on the state$, the entire content would be re-rendered. There is no benefit in doing that.

So my recommendation would be to expose these properties directly under this directive.

loaderType
noData message

Not sure I follow - If we emit again, presumably something has changed and we'd want to rerender. How does exposing a subset help there, anyway? I guess we could keep changing config and keep regenerating the success state, but that seems exceedingly rare and not how we'd use this anyway (and it'd still work).

[anandtiwary]

Yes it will, we're inside ngOnChanges (the condition checks if there is a data input, not wether it changed)

Oh Yeah, I missed that. But any property change of this config would not be propagated unless we change the obj ref.

Not sure I follow - If we emit again, presumably something has changed and we'd want to rerender. How does exposing a subset help there, anyway? I guess we could keep changing config and keep regenerating the success state, but that seems exceedingly rare and not how we'd use this anyway (and it'd still work).

Emitting on state$ on config change doesn't make sense to me. state$ would re-render the entire content template and config doesn't affect that. We can keep them outside and yet achieve the same results

[aaron-steinfeld]

Oh Yeah, I missed that. But any property change of this config would not be propagated unless we change the obj ref.

Which object ref? This emits a new value on wrapperParamsSubject, which was passed via injection to the wrapper.

Emitting on state$ on config change doesn't make sense to me. state$ would re-render the entire content template and config doesn't affect that. We can keep them outside and yet achieve the same results

It would only re-render the content template in the case where we've already gotten data and rendered it, then changed the loader config later. In that case, from a quick look I think we'd end up with a new context (but the same template ref). Yeah, we could address it but that's a serious corner case.

[aaron-steinfeld]

How is this different than icon? I think I missed the context of this change

[anandtiwary]

This one, I was not very keen on. But Michael had the new loaders as gifs. So, we would have to use image. ht-icon won't work as per Alok. I think he tried.

[aaron-steinfeld]

Do you remember why it wouldn't work? I think icon already handles svgs and font ligatures, it just may need some changes to add gif support.

[anandtiwary]

I don't remember. @itssharmasandeep Can we try using ht-icons for this?

[aaron-steinfeld]

lgtm; will let @anandtiwary take a pass

[aaron-steinfeld]

I went back and forth which order these two should come in. I think I'm leaning towards a configured error description should take precedence over one from an error itself. We can leave it as is for now, may change later.

[itssharmasandeep]

Okay, I was also confused about that!!

[anandtiwary]

We could just set the image path here. no need to maintain currentLoaderType

[itssharmasandeep]

#1217 (comment)

We needed this, as if someone passes the undefined to loaderType then it will come to ngOnChanges, now with the previous approach, the image path will be set to ImageAssetPath.LoaderSpinner since we're returning as default from switch case, but the loaderType will be undefined only and it will throw an error for ngClass,
Because we're doing [ngClass]="[this.loaderType]"

So I needed to reassign this var.

[anandtiwary]

LGTM

[github-actions]

Unit Test Results

    4 files  ±0  272 suites  +1   19m 20s ⏱️ + 1m 29s
973 tests +4  973 ✔️ +4  0 💤 ±0  0 ❌ ±0 
980 runs  +4  980 ✔️ +4  0 💤 ±0  0 ❌ ±0 

Results for commit de79bb9. ± Comparison against base commit 981e14b.