Skip to content
This repository has been archived by the owner on Aug 1, 2022. It is now read-only.

Extend Avatar with new fallback data #221

Merged
merged 7 commits into from Mar 13, 2020
Merged

Extend Avatar with new fallback data #221

merged 7 commits into from Mar 13, 2020

Conversation

MeBrei
Copy link
Contributor

@MeBrei MeBrei commented Mar 12, 2020

With #218 avatar fallbacks are provided for identity and can be queried with a handle. This PR extends the Avatar component to use the fallback avatar or query it if a handle is provided. This prepares #216 and #211.

Note that most of the Avatars in the app are currently still using hardcoded imgUrl which should be replaced when refactoring the components.

@MeBrei MeBrei self-assigned this Mar 12, 2020
@MeBrei MeBrei added this to In progress in weekly via automation Mar 12, 2020
@xla xla added feature Something that doesn't exist yet and removed improvement labels Mar 12, 2020
@xla xla requested review from rudolfs and sarahscott March 12, 2020 15:46
@MeBrei MeBrei requested a review from xla March 13, 2020 09:36
@MeBrei MeBrei marked this pull request as ready for review March 13, 2020 09:48
@MeBrei
Copy link
Contributor Author

MeBrei commented Mar 13, 2020

Once we also have the fallback avatars for Orgs I am hoping that the Avatar component can be refactored so that the full Identity or Org data is passed as one prop and Avatar knows how to handle it.

@xla
Copy link
Contributor

xla commented Mar 13, 2020

Once we also have the fallback avatars for Orgs I am hoping that the Avatar component can be refactored so that the full Identity or Org data is passed as one prop and Avatar knows how to handle it.

@MeBrei Generally the surface should only expect the minimum amount of information needed to fullfill it's purpose. Passing entire objects lends itself to deep coupling. Instead it should only require what is necesary to render itself correctly.

weekly automation moved this from In progress to In review Mar 13, 2020
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

Looks good, not super certain about the inline requests in the primitve.

ui/DesignSystem/Primitive/Avatar.svelte Outdated Show resolved Hide resolved
ui/DesignSystem/Primitive/Avatar.svelte Show resolved Hide resolved
weekly automation moved this from In review to Approved Mar 13, 2020
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

(╹◡╹)♡

@MeBrei MeBrei merged commit fc2f7bd into master Mar 13, 2020
weekly automation moved this from Approved to Done Mar 13, 2020
@MeBrei MeBrei deleted the merle/extend-avatar branch March 13, 2020 14:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Something that doesn't exist yet
Projects
No open projects
weekly
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants