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.

Sign up for GitHub

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

Improvements to the Avatar component #57

Merged
merged 9 commits into from
Aug 10, 2023
Merged

Improvements to the Avatar component #57

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2178e9e
Add lazy loading to image
Aug 10, 2023
2dcbd49
Allow Avatar to be a button
Aug 10, 2023
f3aa2cb
Improve avatar stories
Aug 10, 2023
87b22f7
Remove 'as' prop
Aug 10, 2023
aa77654
Only use :disabled pseudo class
Aug 10, 2023
99b3e16
Improve button avatar detection
Aug 10, 2023
d8d9cde
Better avatar documentation
Aug 10, 2023
25a0d15
Apply suggestions for JSDoc comments
germain-gg Aug 10, 2023
2d454e2
Change wording
germain-gg Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/components/Avatar/Avatar.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ limitations under the License.
user-select: none;
}

button.avatar {
/**
* The avatar can be a button element, we need to reset its style
*/
padding: 0;
border: 0;
appearance: none;
cursor: pointer;
}

button.avatar:disabled {
cursor: not-allowed;
}

.avatar,
.image {
aspect-ratio: 1 / 1;
Expand Down
34 changes: 34 additions & 0 deletions src/components/Avatar/Avatar.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ Square.args = {
type: "square",
};

export const Button = Template.bind({});
Button.args = {
type: "round",
as: "button",
onClick: () => console.log("clicked!"),
};

export const NoImageFallback = Template.bind({});
NoImageFallback.args = {
src: "",
Expand All @@ -56,3 +63,30 @@ LargeNoImageFallback.args = {
src: "",
size: "128px",
};

const ImageLessCollection: StoryFn<typeof AvatarComponent> = (args) => (
<>
<AvatarComponent {...args} id="1" />
&nbsp;
<AvatarComponent {...args} id="2" />
&nbsp;
<AvatarComponent {...args} id="3" />
&nbsp;
<AvatarComponent {...args} id="4" />
&nbsp;
<AvatarComponent {...args} id="5" />
&nbsp;
<AvatarComponent {...args} id="6" />
&nbsp;
<AvatarComponent {...args} id="7" />
&nbsp;
<AvatarComponent {...args} id="8" />
&nbsp;
</>
);

export const AllAvatars = ImageLessCollection.bind({});
AllAvatars.args = {
src: "",
size: "36px",
};
125 changes: 89 additions & 36 deletions src/components/Avatar/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,67 @@ import { SuspenseImg } from "../../utils/SuspenseImg";
import styles from "./Avatar.module.css";
import { useIdColorHash } from "./useIdColorHash";

type AvatarProps = JSX.IntrinsicElements["span"] & {
type AvatarProps = (
| JSX.IntrinsicElements["button"]
| JSX.IntrinsicElements["span"]
) & {
/**
* The avatar image URL
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
*/
src?: React.ComponentProps<typeof SuspenseImg>["src"];
/**
* The Matrix ID, Room ID, or Alias to generate the color when no image source
* is provided. Also used as a fallback when name is empty
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
*/
id: string;
/**
* The name used for the initial letter displayed when no image source is provided
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
*/
name: string;
/**
* Defines the avatar type, typically round, square is usually for spaces
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
* @default "round"
*/
type?: "square" | "round";
/**
* The avatar size
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
*/
size?: CSSStyleDeclaration["height"];
/**
* On click handler, will turn the avatar into a button
*/
onClick?: (e: React.MouseEvent) => void;
/**
* Key down handler, will turn the avatar into a button
*/
onKeyDown?: (e: React.KeyboardEvent) => void;
/**
* Key up handler, will turn the avatar into a button
*/
onKeyUp?: (e: React.KeyboardEvent) => void;
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
/**
* Callback when the image has failed to load
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
*/
onError?: React.ComponentProps<typeof SuspenseImg>["onError"];
};

export const Avatar = forwardRef<HTMLSpanElement, AvatarProps>(function Avatar(
/**
* Some props warrant that the avatar become a button for accessibility purposes
* @param props Avatar props
* @returns whether the avatar should be a button or not
*/
function shouldBeAButton(props: Partial<AvatarProps>): boolean {
return !!(props.onClick || props.onKeyDown || props.onKeyUp);
}

/**
* Avatar component that will fallback to an initial letter over a coloured
* background if no source is provided or if the source has failed to load
germain-gg marked this conversation as resolved.
Show resolved Hide resolved
*/
export const Avatar = forwardRef<
HTMLSpanElement | HTMLButtonElement,
Comment on lines +81 to +82
Copy link
Member

Choose a reason for hiding this comment

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

Could this please be generic depending on onClick's presence or as so we don't need to do type assertions on the caller side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think you have access to the ref right here. How would you express that?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this gets a lot trickier with forwardRef but I think bad types here will yield landmines and footguns for callers who are unfamiliar with compound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this expresses the reality of the ref, it can either be a button or a span. I can make it more generic and say HTMLElement if you think that brings more value 🤷

Copy link
Member

@t3chguy t3chguy Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you document it at least onClick ? button : span (but in English)? Is the intention for the storybook to be the developer documentation too? Compound seems to be lacking jsdoc or any documentation for what props mean. Like type=square doesn't say anything about it being a rounded square, onClick doesn't say it'll change the underlying element etc. The overall component doesn't say what its for, that it generates a fallback avatar. I think the lack of documentation puts us in a far worse place than using BaseAvatar in react-sdk.

I think adding JSDoc (and maybe exposing it into Storybook) for intellisense and other IDE tools to pick up would be crucial for making the best (safe) use of the components Compound brings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added JSDoc comments, it was definitely lacking, and I will actually document all the other components with JSDoc comment to make sure this practise spreads across the codebase. Thank you for calling that out.

We would likely not ever document implementation details like "square has rounded corners". But as discussed out of band, documenting the underlying element used, can be greatly beneficial from an a11y point of view.

AvatarProps
>(function Avatar(
{
src,
id,
Expand All @@ -47,39 +98,41 @@ export const Avatar = forwardRef<HTMLSpanElement, AvatarProps>(function Avatar(
const hash = useIdColorHash(id);
const fallbackInitial = <>{getInitialLetter(name)}</>;

return (
<span
ref={ref}
role="img"
title={id}
{...props}
aria-label=""
data-type={type}
data-color={hash}
className={classnames(styles.avatar, className)}
style={
{
...style,
"--cpd-avatar-size": size,
} as React.CSSProperties
}
>
{!src ? (
fallbackInitial
) : (
<Suspense fallback={fallbackInitial}>
<SuspenseImg
src={src}
className={classnames(styles.image)}
data-type={type}
style={style}
width={size}
height={size}
title={id}
onError={onError}
/>
</Suspense>
)}
</span>
return React.createElement(
shouldBeAButton(props) ? "button" : "span",
{
ref,
role: "img",
title: id,
"aria-label": "",
...props,
"data-type": type,
"data-color": hash,
className: classnames(styles.avatar, className),
style: {
...style,
"--cpd-avatar-size": size,
} as React.CSSProperties,
},
[
<>
{!src ? (
fallbackInitial
) : (
<Suspense fallback={fallbackInitial}>
<SuspenseImg
src={src}
className={classnames(styles.image)}
data-type={type}
style={style}
width={size}
height={size}
title={id}
onError={onError}
/>
</Suspense>
)}
</>,
],
);
});
1 change: 1 addition & 0 deletions src/utils/SuspenseImg.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export const SuspenseImg: React.FC<SuspenseImgProps> = ({ src, ...props }) => {
imgCache.read(src);
return (
<img
loading="lazy"
alt=""
src={src}
crossOrigin="anonymous"
Expand Down