goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.25k stars 137 forks source link

[Avatar] Dont Show Avatar after fetching user Data #338

Closed Laguilavo10 closed 3 weeks ago

Laguilavo10 commented 1 month ago

Please provide the environment you discovered this bug in.

Development and Production

Which area/package is the issue in?

avatar

Description

I have a component that fetches some data about the user. After retrieving the data, I want to display the profile picture using the Avatar component. However, it shows the fallback image instead. How can I refresh it?

This code after the fetch continue showing the fallback. code

Please provide the exception or error you saw

No response

Other information

No response

I would be willing to submit a PR to fix this issue

Laguilavo10 commented 1 month ago

I added a conditional validation, it works. However, it would be great if the component could do it by itself.

code1

ajitzero commented 1 month ago

The avatar component (and all others) use ChangeDetection.OnPush, so we need to update the reference to the input. Mutating an existing object will not trigger a new render for the component.

Edit: e.g.

const { display_name, images } = userData;
this.user = {
  name: display_name,
  picture: images[0].url,
};

This should work for your example where you are not using OnPush for HeaderComponent. If you do (better perf), then this.user should be a BehaviourSubject (with an async pipe in template) or a WritableSignal.

Laguilavo10 commented 1 month ago

The avatar component (and all others) use ChangeDetection.OnPush, so we need to update the reference to the input. Mutating an existing object will not trigger a new render for the component.

Edit: e.g.

const { display_name, images } = userData;
this.user = {
  name: display_name,
  picture: images[0].url,
};

This should work for your example where you are not using OnPush for HeaderComponent. If you do (better perf), then this.user should be a BehaviourSubject (with an async pipe in template) or a WritableSignal.

Hi @ajitzero , I changed it to use a BehaviourSubject, but it is still not working.

code2

I tried with


 const { display_name, images } = userData;
 this.user = {
   name: display_name,
   picture: images[0].url,
 };

and didnt work too. What I have wrong?

P.D: I am new with angular, so I`m learning how it works

ajitzero commented 1 month ago

No worries. When using a subject, we should use the async pipe to always get the latest value. If you are using Angular v16 or above, then we can also convert it to a signal.

So either: (old way, before signals, before Angular v16)

@Component({
  imports: [AsyncPipe, NgIf, /* ... */],
})
<ng-container *ngIf="(user | async) as _user">
  <img [src]="_user.picture">
</ng-container>

Or: (new way, with signals, Angular v16 and above)

user = signal({ name: '', picture: '' })
//...
this.user.set({ name: display_name, picture: images[0].url });
<img [src]="user().picture">

Signals make this much simpler to write and maintain in your use-case here. Do check your Angular version in package.json file for what works for you.

Laguilavo10 commented 1 month ago

Thanks for the explanation @ajitzero . I had tried to use the signals now, but it did not work yet with the avatar component, it continued to use the fallback. This is the code code4

The image tag that is outside of it will show normal, but the image tag inside of hlm-avatar used the fallback

image

goetzrobin commented 1 month ago

You are missing the hlmAvatarImage directive in the last example

Laguilavo10 commented 1 month ago

You are missing the hlmAvatarImage directive in the last example

Ok, I added it now, but it does not work yet.

jackyshows commented 1 month ago

@ajitzero @goetzrobin I tried to replicate @Laguilavo10's approach, and it seems he was correct—the issue persists even with ChangeDetection.OnPush or with observables/signals.

I have managed to implement a solution, but I won't open a PR until I get your feedback on whether the code I've written is up to standard. While ( i think :) ) I'm good in Angular, I acknowledge that my expertise may not be at your level.

Here’s the approach I used:

  1. For testing purposes, I created an additional hml-avatar component in the avatar.preview.ts file. This component uses a signal to simulate an HTTP call to provide the src value.

carbon (21)

  1. To make the image reactive, I added a MutationObserver to the brn-avatar-image.directive.ts file. Here’s the relevant code:

carbon (22)

With this implementation, if an incorrect URL is set in the user.picture field, the image disappears briefly (about half a second) and still shows the avatar fallback image. While this is not a complete solution, I hope it can provide some inspiration for a more robust approach.

ajitzero commented 1 month ago

@jackyshows Do you have a reproduction URL or a repo that we could clone?

jackyshows commented 1 month ago

@jackyshows Do you have a reproduction URL or a repo that we could clone?

@ajitzero can you see my forked repo? If so, the branch is: fix/avatar-do-not-show-on-fetch

Repo's link: https://github.com/jackyshows/spartan/tree/fix/avatar-do-not-show-on-fetch

ajitzero commented 1 month ago

I'm able to reproduce the issue separately. My first thought is that we should reset the error & loaded signals whenever src changes.

Let me try it out. If it doesn't work, then mutationObserver seems reasonable.

ajitzero commented 1 month ago

So the fix appears to be even simpler than the src thing I mentioned earlier (which didn't work). The issue was that when the image loads correctly later, our internal error signal is still falsy due to the initial error.

Fix: The error and loaded events are mutually exclusive, so we can update the condition for canShow instead and drop the error signal entirely.

import { Directive, HostListener, computed, signal } from '@angular/core';

@Directive({
    selector: 'img[brnAvatarImage]',
    standalone: true,
    exportAs: 'avatarImage',
})
export class BrnAvatarImageDirective {
-   private readonly error = signal(false);
    private readonly loaded = signal(false);

    @HostListener('error')
    private onError() {
-       this.error.set(true);
+       this.loaded.set(false);
    }

    @HostListener('load')
    private onLoad() {
        this.loaded.set(true);
    }

-   canShow = computed(() => {
-       return this.loaded() && !this.error();
-   });
+   canShow = computed(() => this.loaded());
}

canShow is just loaded itself.

@goetzrobin Do you know if there is any other special edge case for which we need to track error state? The current tests are passing with my suggested changes above. (nx test ui-avatar-brain)

@jackyshows Feel free to add this fix instead as you already have some changes in your branch. Let's just wait for some confirmation from Robin. If you're unavailable, I'll try to pick this up later this weekend.

ajitzero commented 1 month ago

https://github.com/user-attachments/assets/1bf6b39e-f2ce-443c-92b5-14e5bf6ede12

ajitzero commented 1 month ago
jackyshows commented 1 month ago

@ajitzero, thank you! It works. I can create the PR, but let's wait for @goetzrobin first.

goetzrobin commented 1 month ago

@ajitzero @jackyshows let's do it! thanks for figuring this out!