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

feat: New Component Avatar Group [HOMER-2118] #2518

Merged
merged 26 commits into from
Jul 7, 2023

Conversation

Lelith
Copy link
Collaborator

@Lelith Lelith commented Jul 4, 2023

Purpose of PR

Introduces a new component for grouping Avatars. It comes in two sizes and two different designs (stacked and spaced). It will show a maximum of 3 Avatar children, the rest of the children get rendered as a Menu.
The Avatar components require an alt-text which will be displayed as the user name.

image image

@Lelith Lelith requested a review from a team as a code owner July 4, 2023 12:57
@Lelith Lelith requested a review from bgutsol July 4, 2023 12:57
@changeset-bot
Copy link

changeset-bot bot commented Jul 4, 2023

⚠️ No Changeset found

Latest commit: c567ffd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jul 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
forma-36 ✅ Ready (Inspect) Visit Preview Jul 7, 2023 2:32pm

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
CommonJS 147.83 KB (0%) 3 s (0%) 135 ms (+72.66% 🔺) 3.1 s
Module 144.08 KB (-0.01% 🔽) 2.9 s (-0.01% 🔽) 69 ms (-10.49% 🔽) 3 s

Copy link
Contributor

@miguelcrespo miguelcrespo left a comment

Choose a reason for hiding this comment

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

I also don't see the new component in the preview: https://forma-36-git-feat-avatargrouphomer-2118.colorfuldemo.com/components/avatar

Is this expected?

Comment on lines 115 to 135
### Variants

The group can be rendered in two variations

- **spaced** - default variant
- **stacked**

```jsx file=./examples/AvatarGroupVariantExample.tsx

```

### Sizes

The group can be rendered in two different sizes

- **Small** - 24px
- **Medium** - 32px, default size

```jsx file=./examples/AvatarGroupSizeExample.tsx

```
Copy link
Contributor

Choose a reason for hiding this comment

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

question: based on the example of the Avatar shouldn't this headings be under ## Examples ?

It also doesn't have the import section

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

@denkristoffer denkristoffer left a comment

Choose a reason for hiding this comment

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

The docs are broken: https://forma-36-git-feat-avatargrouphomer-2118.colorfuldemo.com/components/avatar

At least one of the errors is because the new component has not been added to the SandpackRenderer and LiveEditor.

Copy link
Contributor

@bgutsol bgutsol left a comment

Choose a reason for hiding this comment

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

Other than what I've commented, looks good!

Also make sure to add AvatarGroup import here and pass it to the liveProviderScope

SandpackRenderer though should work fine when the new version gets published.

@@ -97,3 +97,39 @@ When the avatar image is loading, a loading skeleton will be shown automatically
## Accessibility

Make sure to pass a fitting `alt` property, especially if the avatar is used by itself without the user's name next to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's follow the same structure we have in Button for example, which also has a ButtonGroup. And by the same structure I mean separate folder avatar/src/AvatarGroup where you can have implementation, styles, README, etc.

Comment on lines 115 to 135
### Variants

The group can be rendered in two variations

- **spaced** - default variant
- **stacked**

```jsx file=./examples/AvatarGroupVariantExample.tsx

```

### Sizes

The group can be rendered in two different sizes

- **Small** - 24px
- **Medium** - 32px, default size

```jsx file=./examples/AvatarGroupSizeExample.tsx

```
Copy link
Contributor

Choose a reason for hiding this comment

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


import { cx } from 'emotion';

export type Variant = 'stacked' | 'spaced';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we export it I'd give it a more specific name

Suggested change
export type Variant = 'stacked' | 'spaced';
export type AvatarGroupVariant = 'stacked' | 'spaced';


export interface AvatarGroupProps extends CommonProps {
spacing?: SpacingTokens;
className?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

className is already assigned from CommonProps

Suggested change
className?: string;

Comment on lines 22 to 113
const renderAvatars = (
children: AvatarGroupProps['children'],
size: AvatarGroupProps['size'],
variant: AvatarGroupProps['variant'],
) => {
const styles = getAvatarGroupStyles(size);

if (React.Children.count(children) > 3) {
return (
<>
{React.Children.map(children, (child, index) => {
if (index < 2) {
return React.cloneElement(child as React.ReactElement, {
key: `avatar-rendered-${index}`,
size: size,
className: cx((child as React.ReactElement).props.className, {
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
}),
});
}
})}
<Menu placement="bottom-end">
<Menu.Trigger>
<button
type="button"
className={cx(
{
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
},
styles.moreAvatarsBtn,
)}
>
{React.Children.count(children) - 2}
</button>
</Menu.Trigger>
<Menu.List>
{React.Children.toArray(children)
.slice(2)
.map((child, index) => {
return (
<Menu.Item
className={styles.moreAvatarsItem}
key={`avatar-${index}`}
>
{React.cloneElement(child as React.ReactElement, {
key: `avatar-menuitem-${index}`,
size: 'tiny',
})}
{(child as React.ReactElement).props.alt}
</Menu.Item>
);
})}
</Menu.List>
</Menu>
</>
);
}
return React.Children.map(children, (child, index) => {
return React.cloneElement(child as React.ReactElement, {
key: `avatar-rendered-${index}`,
size: size,
className: cx((child as React.ReactElement).props.className, {
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
}),
});
});
};

function _AvatarGroup(
{
children,
className,
size = 'medium',
variant = 'spaced',
testId = 'cf-ui-avatar-group',
}: AvatarGroupProps,
forwardedRef: React.Ref<HTMLDivElement>,
) {
return (
<Flex
flexDirection="row"
className={cx(className)}
data-test-id={testId}
ref={forwardedRef}
>
{renderAvatars(children, size, variant)}
</Flex>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can simplify it a bit. Sorry for nitpicking, just wanted to share the idea.

Suggested change
const renderAvatars = (
children: AvatarGroupProps['children'],
size: AvatarGroupProps['size'],
variant: AvatarGroupProps['variant'],
) => {
const styles = getAvatarGroupStyles(size);
if (React.Children.count(children) > 3) {
return (
<>
{React.Children.map(children, (child, index) => {
if (index < 2) {
return React.cloneElement(child as React.ReactElement, {
key: `avatar-rendered-${index}`,
size: size,
className: cx((child as React.ReactElement).props.className, {
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
}),
});
}
})}
<Menu placement="bottom-end">
<Menu.Trigger>
<button
type="button"
className={cx(
{
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
},
styles.moreAvatarsBtn,
)}
>
{React.Children.count(children) - 2}
</button>
</Menu.Trigger>
<Menu.List>
{React.Children.toArray(children)
.slice(2)
.map((child, index) => {
return (
<Menu.Item
className={styles.moreAvatarsItem}
key={`avatar-${index}`}
>
{React.cloneElement(child as React.ReactElement, {
key: `avatar-menuitem-${index}`,
size: 'tiny',
})}
{(child as React.ReactElement).props.alt}
</Menu.Item>
);
})}
</Menu.List>
</Menu>
</>
);
}
return React.Children.map(children, (child, index) => {
return React.cloneElement(child as React.ReactElement, {
key: `avatar-rendered-${index}`,
size: size,
className: cx((child as React.ReactElement).props.className, {
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
}),
});
});
};
function _AvatarGroup(
{
children,
className,
size = 'medium',
variant = 'spaced',
testId = 'cf-ui-avatar-group',
}: AvatarGroupProps,
forwardedRef: React.Ref<HTMLDivElement>,
) {
return (
<Flex
flexDirection="row"
className={cx(className)}
data-test-id={testId}
ref={forwardedRef}
>
{renderAvatars(children, size, variant)}
</Flex>
);
}
function _AvatarGroup(
{
children,
className,
size = 'medium',
variant = 'spaced',
testId = 'cf-ui-avatar-group',
}: AvatarGroupProps,
forwardedRef: React.Ref<HTMLDivElement>,
) {
const styles = getAvatarGroupStyles(size);
const childrenArray = React.Children.toArray(children);
const childrenToRenderCount = childrenArray.length > 3 ? 2 : 3;
const childrenToRender = childrenArray.slice(0, childrenToRenderCount);
const childrenInMenu = childrenArray.slice(childrenToRenderCount);
return (
<Flex
flexDirection="row"
className={cx(className)}
data-test-id={testId}
ref={forwardedRef}
>
{childrenToRender.map((child, index) => {
return React.cloneElement(child as React.ReactElement, {
key: `avatar-rendered-${index}`,
size: size,
className: cx((child as React.ReactElement).props.className, {
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
}),
});
})}
{childrenInMenu.length > 0 && (
<Menu placement="bottom-end">
<Menu.Trigger>
<button
type="button"
className={cx(
{
[styles.avatarStacked]: variant === 'stacked',
[styles.avatarSpaced]: variant === 'spaced',
},
styles.moreAvatarsBtn,
)}
>
{childrenInMenu.length}
</button>
</Menu.Trigger>
<Menu.List>
{childrenInMenu.map((child, index) => {
return (
<Menu.Item
className={styles.moreAvatarsItem}
key={`avatar-${index}`}
>
{React.cloneElement(child as React.ReactElement, {
key: `avatar-menuitem-${index}`,
size: 'tiny',
})}
{(child as React.ReactElement).props.alt}
</Menu.Item>
);
})}
</Menu.List>
</Menu>
)}
</Flex>
);
}

import { cx } from 'emotion';

export interface AvatarGroupProps extends CommonProps {
spacing?: SpacingTokens;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the spacing prop being used anywhere, should it be?

packages/components/avatar/src/AvatarGroup/AvatarGroup.tsx Outdated Show resolved Hide resolved
@denkristoffer
Copy link
Collaborator

Can you please bump the package.json version? You can do it with npm run-script prerelease or manually.

@miguelcrespo
Copy link
Contributor

Can you please bump the package.json version? You can do it with npm run-script prerelease or manually.

@denkristoffer Done

@denkristoffer denkristoffer merged commit ca6c2dd into main Jul 7, 2023
3 checks passed
@denkristoffer denkristoffer deleted the feat/Avatar_Group_HOMER-2118 branch July 7, 2023 16:57
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.

5 participants