Conversation
finalight
force-pushed
the
feature/next-avatar
branch
16 times, most recently
from
029fbaf to
bf2245b
Compare
| export type AvatarSizeVariant = typeof AvatarSizeVariant[number]; | ||
|
|
||
| export interface AvatarProps extends React.HTMLAttributes<HTMLDivElement> { | ||
| variant: AvatarBackgroundColorVariant; |
There was a problem hiding this comment.
this should also be optional since we should define a default based on the Figma
| initials?: string; | ||
| } | ||
|
|
||
| export const Avatar = ({ variant, size, initials, ...props }: AvatarProps) => { |
There was a problem hiding this comment.
based on Figma, both variant and size have default values. we can define them here instead of adding logic on the passed props below
There was a problem hiding this comment.
i've passed in the default value for the props
| const getAvatarBackgroundColor = (variant: AvatarBackgroundColorVariant) => { | ||
| if (!(variant in avatarBackgroundColor)) { | ||
| console.warn( | ||
| `${variant} is not a valid variant for the backgroud color, default will be used` |
There was a problem hiding this comment.
| `${variant} is not a valid variant for the backgroud color, default will be used` | |
| `${variant} is not a valid variant for the background color, default will be used` |
| const avatarSize: { | ||
| [variant in AvatarSizeVariant[number]]: number; | ||
| } = { | ||
| ['large']: 60, | ||
| ['medium']: 40, | ||
| }; | ||
|
|
||
| const mobileAvatarSize: { | ||
| [variant in AvatarSizeVariant[number]]: number; | ||
| } = { | ||
| ['large']: 40, | ||
| ['medium']: 32, | ||
| }; |
There was a problem hiding this comment.
can we do something like this for both medium and large styles:
const mediumAvatarSizeStyle = `
width: 40px;
height: 40px;
@media (max-width: ${Breakpoints.large}) {
width: 32px;
height: 32px;
}
`;
and map the style with the size so we eliminate the other called functions as much as we can inside the styled component.
There was a problem hiding this comment.
i did some refactoring of it, you can take a look
also, i've made sure that the snapshot didn't get updated to maintain the integrity of the outcome
finalight
force-pushed
the
feature/next-avatar
branch
from
bf2245b to
47c4a2a
Compare
| ...props | ||
| }: AvatarProps) => { | ||
| return ( | ||
| <AvatarStyle variant={variant} size={size ?? 'large'} {...props}> |
There was a problem hiding this comment.
| <AvatarStyle variant={variant} size={size ?? 'large'} {...props}> | |
| <AvatarStyle variant={variant} size={size} {...props}> |
we should be able to remove this because of the default value above.
| return ( | ||
| <AvatarStyle variant={variant} size={size ?? 'large'} {...props}> | ||
| <Typography | ||
| variant={size === null || size === 'medium' ? 'body1' : 'headline6'} |
There was a problem hiding this comment.
| variant={size === null || size === 'medium' ? 'body1' : 'headline6'} | |
| variant={size === 'medium' ? 'body1' : 'headline6'} |
we should be able to remove this because of the default value above.
finalight
force-pushed
the
feature/next-avatar
branch
from
47c4a2a to
630a933
Compare
2 participants
No description provided.