Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Display avatar after fetching user data #342

Closed
wants to merge 4 commits into from

Conversation

jackyshows
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

Which package are you modifying?

  • accordion
  • alert
  • alert-dialog
  • aspect-ratio
  • avatar
  • badge
  • button
  • calendar
  • card
  • checkbox
  • collapsible
  • combobox
  • command
  • context-menu
  • data-table
  • date-picker
  • dialog
  • dropdown-menu
  • hover-card
  • icon
  • input
  • label
  • menubar
  • navigation-menu
  • pagination
  • popover
  • progress
  • radio-group
  • scroll-area
  • select
  • separator
  • sheet
  • skeleton
  • slider
  • sonner
  • spinner
  • switch
  • table
  • tabs
  • textarea
  • toast
  • toggle
  • tooltip
  • typography

What is the current behavior?

(#338)

What is the new behavior?

The avatar component no longer has an error state; it now only handles a loaded state. It correctly updates the user avatar when the src attribute changes, without always showing the fallback avatar. Inside the @HostListener('error') method, it sets the loaded state to false, and the canShow flag only checks if the avatar is loaded or not.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

@ajitzero ajitzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the PR!

this._renderer2.removeStyle(tooltip, 'display');
}else{
this._renderer2.setStyle(tooltip, 'display', 'none');
}
Copy link
Contributor

@ajitzero ajitzero Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated, please rebase from the main branch and this should get removed automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajitzero this is my first time working on an open source project. You're right, I'm not sure why this commit is here, and I don't know how to fix it. I tried to rebase from the main branch, but it doesn't seem to work; I'm surely doing something wrong. Feel free to delete this PR, and I'll delete my forked repo and create a new, clean PR. Alternatively, if you prefer, you can create a new one yourself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! git can get confusing with forks and whatnot. I see you have 3 extra commits in this PR https://github.com/goetzrobin/spartan/pull/342/commits - I'm not sure what could have caused this either. Could you please try removing these last three commits?

Try these CLI commands:

# go to your repo/fork's branch with the changes
git checkout main

# remove the last N (here, 3) commits: git reset --hard HEAD~N
git reset --hard HEAD~3

# Update your fork on GitHub forcibly and overwrite it.
# We don't do this for actual projects, but is common for cleaning up PRs.
git push --force

To be cautious though, I would suggest running the reset command without the --hard flag. This will only "undo" the changes and keep them available locally. You can then decide what to skip manually.

@jackyshows jackyshows closed this Jul 28, 2024
@jackyshows jackyshows reopened this Jul 28, 2024
@goetzrobin goetzrobin closed this Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants