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

Improvements to the Avatar component #57

Merged
merged 9 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
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, if any.
*/
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.
*/
id: string;
/**
* The name used for the initial letter displayed when no image source is provided.
*/
name: string;
/**
* Defines the avatar type, typically round, square is usually for spaces.
* @default "round"
*/
type?: "square" | "round";
/**
* The avatar size in CSS units, e.g. `"24px"`.
*/
size?: CSSStyleDeclaration["height"];
/**
* On click handler, will turn the avatar into a button element.
*/
onClick?: (e: React.MouseEvent) => void;
/**
* Key down handler, will turn the avatar into a button element.
*/
onKeyDown?: (e: React.KeyboardEvent) => void;
/**
* Key up handler, will turn the avatar into a button element.
*/
onKeyUp?: (e: React.KeyboardEvent) => void;
/**
* Callback when the image has failed to load.
*/
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.
*/
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

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
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