Few improvements

[rafaelss95]

I'm submitting a ... (check one with "x")

• bug report => search github for a similar issue or PR before submitting
• feature request

Suggestions:

• Use OnPush change detection for performance reasons;
• @Input for changing the "Loading..." text (

  ngx-skeleton-loader/projects/ngx-skeleton-loader/src/lib/ngx-skeleton-loader.html

  Line 7 in 0f26434

  aria-valuetext="Loading..."

  );
• Better typings with strict mode. Currently we can't pass null/undefined properties, for example. A really usual case is the use with AsyncPipe where the options could be undefined. ngAcceptInputType_* static members could help here.

Also, it seems that animation can't accept literal false (

ngx-skeleton-loader/projects/ngx-skeleton-loader/src/lib/ngx-skeleton-loader.component.ts

Line 17 in 0f26434

animation: 'progress' | 'progress-dark' | 'pulse' | 'false' = 'progress';

)

Another thing is that the theme is not compatible with NgStyle, so we can't pass something like {'max-width.px': widthExp} as the current signature is {[k: string]: string;}

I can try to help by sending a PR, if it is desirable. Thanks for this amazing lib :)

[willmendesneto]

Hi @rafaelss95 , thanks for raising this issue.

Ok, so let's try to discuss each of these points since it's an issue covering different points:

• Use OnPush change detection for performance reasons
• @input for changing the "Loading..." text
• Also, it seems that animation can't accept literal false

Nice findings. I'm applying these changes at the moment, thanks for the suggestions!

Another thing is that the theme is not compatible with NgStyle, so we can't pass something like {'max-width.px': widthExp} as the current signature is {[k: string]: string;}

Actually, this is a design decision. Since you can use [theme] for it, there's no reason to support NgStyle via attribute bindings, like the scenario you described. For it you can use something like {"max-width": "100px"} should work as expected and I never received any message with scenarios when a binding in [theme] was required.

Better typings with strict mode...

Could you share a real scenario when you'll have to use AsyncPipe for it and that ngAcceptInputType_* ? I'm just concerned that won't be a realistic scenario and maybe sharing something specific you're facing might help on that.

[willmendesneto]

AT TIME:

• I just released a new version with the suggestions and they're available in ngx-skeleton-loader@2.7.0. More details about the changes in https://github.com/willmendesneto/ngx-skeleton-loader/blob/master/CHANGELOG.md#270---2021-02-06

[rafaelss95]

Hey @willmendesneto sorry for late response. I saw that you made progress with this, and I'd want to say thanks for this awesome lib and for providing a fast feedback on these suggestions.

Btw, could we have a way to pass a default config to the .forRoot()?

ngx-skeleton-loader/projects/ngx-skeleton-loader/src/lib/ngx-skeleton-loader.module.ts

Line 12 in bb7ad60

static forRoot(): ModuleWithProviders<NgxSkeletonLoaderModule> {

It'd be really nice to set a custom loadingText globally, and why not the other properties as well.

---

That being said, let me try to address the remaining two points...

Actually, this is a design decision. Since you can use [theme] for it, there's no reason to support NgStyle via attribute bindings, like the scenario you described. For it you can use something like {"max-width": "100px"} should work as expected and I never received any message with scenarios when a binding in [theme] was required.

Hmm, I don't think my suggestion was clear enough. Let me try to explain better now...

As the [theme] @Input is just a bridge to pass an style via [ngStyle] as we can see here:

ngx-skeleton-loader/projects/ngx-skeleton-loader/src/lib/ngx-skeleton-loader.html

Line 16 in a75ad6c

[ngStyle]="theme"

I don't get why it isn't compatible with NgStyle and that's why I was suggesting to use the same signature of [ngStyle] ({ [klass: string]: any; }) in [theme].

Could you share a real scenario when you'll have to use AsyncPipe for it and that ngAcceptInputType_* ? I'm just concerned that won't be a realistic scenario and maybe sharing something specific you're facing might help on that.

We don't even have to go that far, by using examples with AsyncPipe, just try to use (in a application with strict mode enabled) like in README:

<ngx-skeleton-loader count="5"></ngx-skeleton-loader>

You'll receive Type 'string' is not assignable to type 'number'.

[willmendesneto]

Thanks for elaborate on the issue. Maybe we should split that in few other issues to make it easier to manage, but definitely some improvements to be added!

I’m not sure if I’ll have some time these days to apply that, but pull requests are more than welcome (It’s totally fine if you can’t raise them, I can add them)!

[willmendesneto]

Sharing some updates:

• strict types are now available on the package;
• theme attribute is now following the same types asngStyle. So now it can be used as style attributes as well, having both the same signature.

[willmendesneto]

I'm closing this issue for now since it was a lot of issues in a single one. Please feel free to open a single issue, so we could discuss more and see if it's valuable for the module!

And thanks again for raising those points 🎉

[rafaelss95]

Hey @willmendesneto, thanks for addressing this! I've just updated right now and I've encountered a problem that I reported in #61 and a missing suggestion in #62.