-
Notifications
You must be signed in to change notification settings - Fork 888
refactor(site): Normalize avatar components #5860
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
Conversation
BrunoQuaresma
commented
Jan 25, 2023
- Extended the MuiAvatar for our use case
- Make all avatars look the same and use a common component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new avatar component looks great!
|
||
export interface AvatarDataProps { | ||
title: string | ||
subtitle?: string | ||
highlightTitle?: boolean | ||
link?: string | ||
src?: string | ||
avatar?: React.ReactNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really important, just a thought but would it make sense to make AvatarData
take Avatar
props and pass them along rather than accept avatar
? My reasoning is that it seems unfortunate that there are two ways of doing things like:
<AvatarData src={src} />
and
<AvatarData avatar={<Avatar src={src} />} />
Or alternatively we could always make it so we always have to pass an avatar
, either way works so there is one way of doing things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AvatarData
is a bad name for what it does and not a good abstraction. Wondering if Heading
or DataHeading
would be better with the following interface:
<Heading
title="Bruno"
subtitle="bruno@hotmail.com"
leftElement={<UserAvatar user="{user}" />}
/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree!
return ( | ||
<Avatar colorScheme="darken"> | ||
<AvatarIcon> | ||
<img src={avatarSrc} alt="" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check I follow: the difference between <Avatar src={src} />
and <Avatar><img src={src} /></Avatar>
is that the former shows the icon only and the former shows the icon wrapped in a circle, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is: when you pass the image as a child, it does not add the MuiAvatar-img
class to it so it is just an image inside of a rounded div
without any style. My idea for the AvatarIcon
is to make the image behaves like a MuiIcon
.
Thinking better, I have no idea why I decided to use a child instead of using the AvatarIcon
directly like:
// Instead of
<AvatarIcon>
<img src={avatarSrc} alt="" />
</AvatarIcon>
// it should be
<AvatarIcon src={avatarSrc} />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh ok yeah that makes sense!
@@ -173,7 +173,6 @@ export const GroupPage: React.FC = () => { | |||
<AvatarData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like before the avatars on the group page would overlap a little but now they touch but do not look like they are overlapping. Was that intentional? I think the overlap looks kinda nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'm fixing this.