Skip to content

Conversation

brianacnguyen
Copy link
Contributor

@brianacnguyen brianacnguyen commented Mar 1, 2025

Description

This PR adds the AvatarGroup component to the DSRN

Related issues

Fixes: #371

Manual testing steps

  1. Run yarn storybook:ios from root
  2. Go to Components > AvatarGroup

Screenshots/Recordings

Before

After

Added more sample stories to different Avatar variants
https://github.com/user-attachments/assets/9620df1f-0299-42b4-b347-75482318e65e

AvatarGroup stories
https://github.com/user-attachments/assets/f0662a4c-0d0d-4fef-beb2-a9a423eb5127

Pre-merge author checklist

  • I've followed MetaMask Contributor Docs
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@brianacnguyen brianacnguyen requested a review from a team as a code owner March 1, 2025 02:52
@brianacnguyen brianacnguyen marked this pull request as draft March 1, 2025 02:52
@brianacnguyen brianacnguyen changed the title [DRAFT] Added AvatarGroup [DSRN] Added AvatarGroup Mar 5, 2025
@brianacnguyen brianacnguyen self-assigned this Mar 5, 2025
@brianacnguyen brianacnguyen marked this pull request as ready for review March 5, 2025 08:34
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Looks like you may need to remove the other avatars from this PR?

@brianacnguyen
Copy link
Contributor Author

Looks like you may need to remove the other avatars from this PR?

Nope those changes are enhancements to all Avatars stemming from AvatarGroup requirements so I kept them in here

'0x8AceA3A9748294d1B5C25a08EFE37b756AafDFdd',
'0xEC5CE72f2e18B0017C88F7B12d3308119C5Cf129',
'0xeC56Da21c90Af6b50E4Ba5ec252bD97e735290fc',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these so they can be reused for both AvatarAccount, AvatarGroup, and any other components built from AvatarAccount

'https://trezor.io/favicon.ico',
'https://electrum.org/favicon.ico',
'https://cryptologos.cc/logos/ethereum-eth-logo.svg',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these so they can be reused for both AvatarFavicon, AvatarGroup, and any other components built from AvatarFavicon

'https://cryptologos.cc/logos/ethereum-eth-logo.svg',
'https://cryptologos.cc/logos/solana-sol-logo.svg',
'https://cryptologos.cc/logos/tether-usdt-logo.svg',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these so they can be reused for both AvatarNetwork, AvatarGroup, and any other components built from AvatarNetwork

'https://cryptologos.cc/logos/chainlink-link-logo.svg',
'https://cryptologos.cc/logos/uniswap-uni-logo.svg',
'https://cryptologos.cc/logos/flare-flr-logo.svg',
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these so they can be reused for both AvatarToken, AvatarGroup, and any other components built from AvatarToken

* For internal use only
* @default false
*/
hasSolidBackgroundColor?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are needed for AvatarGroup and Badge

Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: Can we create a ticket or separate PR to add these props to the React component to ensure alignment across our component APIs? I think it's fine to handle the styling logic differently internally within the component.

@georgewrmarshall georgewrmarshall requested a review from Copilot March 7, 2025 21:46
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR introduces the new AvatarGroup component and its supporting utilities, tests, stories, and constants to improve the design system's avatar management capabilities. Key changes include:

  • Adding the AvatarGroup component with overflow handling and configurable props.
  • Creating utility functions for generating Tailwind class names for layout and overflow.
  • Adding comprehensive test cases and Storybook stories across all avatar variants.

Reviewed Changes

File Description
README.md Added comprehensive documentation for AvatarGroup props, usage examples, and guidelines.
AvatarGroup.utilities.ts Introduced utility functions for generating container and overflow text Tailwind class names.
AvatarGroup.types.ts Defined types and enums for AvatarGroup and its variants.
AvatarGroup.tsx Implemented the AvatarGroup component with overflow counter logic and variant rendering.
AvatarGroup.test.tsx Added tests to validate utility functions and proper rendering behavior of AvatarGroup.
AvatarGroup.stories.tsx Created Storybook stories to showcase all avatar variants and different sizes/orientations.
AvatarGroup.constants.ts Defined constants for AvatarGroup including default props and sample data.
Various Avatar* files Updated AvatarAccount, AvatarFavicon, AvatarNetwork, and AvatarToken components and their stories/tests to integrate with new constants and sample data.
storybook.requires.js Registered the new AvatarGroup stories in the Storybook configuration.

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

packages/design-system-react-native/src/components/AvatarGroup/AvatarGroup.constants.ts:14

  • [nitpick] Consider renaming 'MAP_AVATARGROUP_SIZE_SPACEBETWEENAVATARS' to a name that more clearly indicates its purpose (e.g., 'AVATAR_OVERLAP_GAP') to improve clarity and readability.
export const MAP_AVATARGROUP_SIZE_SPACEBETWEENAVATARS: Record<AvatarGroupSize, number> = {

shape = DEFAULT_AVATARNETWORK_PROPS.shape,
fallbackText,
fallbackTextProps,
hasBorder,
Copy link
Preview

Copilot AI Mar 7, 2025

Choose a reason for hiding this comment

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

New props 'hasBorder' and 'hasSolidBackgroundColor' are now passed to AvatarBase without default values. Consider defining default values for these props to ensure consistent styling when they are omitted.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

LGTM! In the future, it would be good to break this PR into two—it's over 1000 lines, and the hasBorder and hasSolidBackgroundColor updates could have been easily separated into their own PRs.

We're also missing stories and README updates for hasBorder and hasSolidBackgroundColor, which should be added in a subsequent PR.

Checked:

  • Stories and controls for AvatarGroup

Comment on lines +41 to +43
return (
<AvatarAccount {...args} address={SAMPLE_AVATARACCOUNT_ADDRESSES[0]} />
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the benefits of storing sample data in constants, but we should also be aware of the tradeoffs. No action items—just noting that there are some downsides.

Pros:

  • Reusability across stories, tests, and documentation
  • Single source of truth for sample data
  • Easier maintenance if addresses need to change
  • Can be used for testing

Cons:

  • Adds indirection—readers need to jump between files
  • May obscure the immediate understanding of what's being demonstrated
  • Could lead to overengineering with too many sample datasets

Let's keep these in mind—maybe a mixed approach would help mitigate this:

// Keep minimal examples direct in stories
export const Default: Story = {
  render: () => (
    <AvatarAccount 
      address="0x9Cbf7c41B7787F6c621115010D3B044029FE2Ce8" 
    />
  )
};

// Use constants for complex scenarios or when reuse is valuable
export const MultipleAddresses: Story = {
  render: () => (
    <View style={{ gap: 16 }}>
      {SAMPLE_AVATARACCOUNT_ADDRESSES.slice(0, 3).map((address) => (
        <AvatarAccount key={address} address={address} />
      ))}
    </View>
  )
};

Copy link
Contributor

@georgewrmarshall georgewrmarshall Mar 7, 2025

Choose a reason for hiding this comment

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

You can also do things like this without any type errors where the variant is token but the sample props are for network

export const Default: Story = {
  render: () => (
    <AvatarGroup
      variant={AvatarGroupVariant.Token}
      avatarPropsArr={SAMPLE_AVATARGROUP_AVATARNETWORKPROPSARR}
    />
  ),
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same thing can be said for our tokens

* For internal use only
* @default false
*/
hasSolidBackgroundColor?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: Can we create a ticket or separate PR to add these props to the React component to ensure alignment across our component APIs? I think it's fine to handle the styling logic differently internally within the component.

type Story = StoryObj<AvatarFaviconProps>;
const storyImageSource: ImageSourcePropType = {
uri: 'https://metamask.github.io/test-dapp/metamask-fox.svg',
uri: SAMPLE_AVATARFAVICON_URIS[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: same comment here about storing sample data in constants. I think using a string here is helpful to instantly show what the value should be.

Suggested change
uri: SAMPLE_AVATARFAVICON_URIS[0],
uri: 'https://metamask.github.io/test-dapp/metamask-fox.svg',

Comment on lines +84 to +93
export const Default: Story = {
args: {
variant: AvatarGroupVariant.Favicon,
size: DEFAULT_AVATARGROUP_PROPS.size,
max: DEFAULT_AVATARGROUP_PROPS.max,
isReverse: DEFAULT_AVATARGROUP_PROPS.isReverse,
twClassName: '',
},
render: (args) => <AvatarGroupStory {...args} />,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: Not sure if this is intended or needs to be fixed but when isReverse is true it aligns differently to the page

Screenshot 2025-03-07 at 2 11 21 PMScreenshot 2025-03-07 at 2 11 15 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the result of row vs row-reverse so it's working as intended

@brianacnguyen
Copy link
Contributor Author

LGTM! In the future, it would be good to break this PR into two—it's over 1000 lines, and the hasBorder and hasSolidBackgroundColor updates could have been easily separated into their own PRs.

We're also missing stories and README updates for hasBorder and hasSolidBackgroundColor, which should be added in a subsequent PR.

Checked:

  • Stories and controls for AvatarGroup

I intentionally did not add stories and README for hasBorder and hasSolidBackgroundColor as those are for internal use only

@brianacnguyen brianacnguyen merged commit d26405a into main Mar 11, 2025
30 checks passed
@brianacnguyen brianacnguyen deleted the dsrn/avatar-group branch March 11, 2025 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[React Native]: Create AvatarGroup component in shared UI component library
2 participants