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

Adding support for framework image component in Radix Avatar #2999

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions .yarn/versions/48669fbc.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
releases:
"@radix-ui/react-avatar": minor
primitives: patch
56 changes: 56 additions & 0 deletions packages/react/avatar/src/Avatar.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,19 @@ import * as Avatar from '@radix-ui/react-avatar';
export default { title: 'Components/Avatar' };

const src = 'https://picsum.photos/id/1005/400/400';
const otherSrc = 'https://picsum.photos/id/1006/400/400';
const srcBroken = 'https://broken.link.com/broken-pic.jpg';

const FakeFrameworkImage = (props: any) => {
console.log(props);

return (
<div>
<img {...props} alt="framework test" data-testid="framework-image-component" />
</div>
);
};

export const Styled = () => (
<>
<h1>Without image & with fallback</h1>
Expand Down Expand Up @@ -33,6 +44,31 @@ export const Styled = () => (
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>

<h1>With image framework component</h1>
<Avatar.Root className={rootClass()}>
<Avatar.Image
className={imageClass()}
alt="John Smith"
onLoadingStatusChange={console.log}
asChild
>
<FakeFrameworkImage src={otherSrc} />
</Avatar.Image>
<Avatar.Fallback className={fallbackClass()}>
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>

<h1>With image framework component & with fallback (but broken src)</h1>
<Avatar.Root className={rootClass()}>
<Avatar.Image className={imageClass()} alt="John Smith" onLoadingStatusChange={console.log}>
<FakeFrameworkImage src={srcBroken} />
</Avatar.Image>
<Avatar.Fallback className={fallbackClass()}>
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>
</>
);

Expand All @@ -58,6 +94,26 @@ export const Chromatic = () => (
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>

<h1>With image framework component</h1>
<Avatar.Root className={rootClass()}>
<Avatar.Image className={imageClass()} alt="John Smith" onLoadingStatusChange={console.log}>
<FakeFrameworkImage src={otherSrc} />
</Avatar.Image>
<Avatar.Fallback className={fallbackClass()}>
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>

<h1>With image framework component & with fallback (but broken src)</h1>
<Avatar.Root className={rootClass()}>
<Avatar.Image className={imageClass()} alt="John Smith" onLoadingStatusChange={console.log}>
<FakeFrameworkImage src={srcBroken} />
</Avatar.Image>
<Avatar.Fallback className={fallbackClass()}>
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>
</>
);
Chromatic.parameters = { chromatic: { disable: false, delay: 1000 } };
Expand Down
65 changes: 64 additions & 1 deletion packages/react/avatar/src/Avatar.test.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { axe } from 'jest-axe';
import type { RenderResult } from '@testing-library/react';
import { render, waitFor } from '@testing-library/react';
import { render } from '@testing-library/react';
import * as Avatar from '@radix-ui/react-avatar';

const ROOT_TEST_ID = 'avatar-root';
const FALLBACK_TEXT = 'AB';
const IMAGE_ALT_TEXT = 'Fake Avatar';
const DELAY = 300;
const FRAMEWORK_IMAGE_TEST_ID = 'framework-image-component';
const FRAMEWORK_IMAGE_ALT_TEXT = 'framework test';

describe('given an Avatar with fallback and no image', () => {
let rendered: RenderResult;
Expand All @@ -33,6 +35,7 @@ describe('given an Avatar with fallback and a working image', () => {
(window.Image as any) = class MockImage {
onload: () => void = () => {};
src: string = '';

constructor() {
setTimeout(() => {
this.onload();
Expand Down Expand Up @@ -101,6 +104,66 @@ describe('given an Avatar with fallback and delayed render', () => {
});
});

describe('given an Avatar with fallback and child image', () => {
let rendered: RenderResult;
let image: HTMLElement | null = null;
const orignalGlobalImage = window.Image;

beforeAll(() => {
(window.Image as any) = class MockImage {
onload: () => void = () => {};
src: string = '';

constructor() {
setTimeout(() => {
this.onload();
}, DELAY);
return this;
}
};
});

afterAll(() => {
window.Image = orignalGlobalImage;
});

beforeEach(() => {
rendered = render(
<Avatar.Root data-testid={ROOT_TEST_ID}>
<Avatar.Image asChild>
<img
src="/test.jpg"
data-testid={FRAMEWORK_IMAGE_TEST_ID}
alt={FRAMEWORK_IMAGE_ALT_TEXT}
/>
</Avatar.Image>
<Avatar.Fallback>{FALLBACK_TEXT}</Avatar.Fallback>
</Avatar.Root>
);
console.log(rendered);
});

it('should render the image after it has loaded', async () => {
image = await rendered.findByRole('img');
expect(image).toBeInTheDocument();
});

it('should have alt text on the image', async () => {
image = await rendered.findByAltText(FRAMEWORK_IMAGE_ALT_TEXT);
expect(image).toBeInTheDocument();
});

it('should render the fallback initially', () => {
const fallback = rendered.queryByText(FALLBACK_TEXT);
expect(fallback).toBeInTheDocument();
});

it('should render the image framework component', () => {
const frameworkImage = rendered.queryByTestId(FRAMEWORK_IMAGE_TEST_ID);
expect(frameworkImage).toBeInTheDocument();
});
});

describe('given an Avatar with an image that only works when referrerPolicy=no-referrer', () => {
let rendered: RenderResult;
const orignalGlobalImage = window.Image;
Expand Down
52 changes: 44 additions & 8 deletions packages/react/avatar/src/Avatar.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import * as React from 'react';
import type { Scope } from '@radix-ui/react-context';
import { createContextScope } from '@radix-ui/react-context';
import { useCallbackRef } from '@radix-ui/react-use-callback-ref';
import { useLayoutEffect } from '@radix-ui/react-use-layout-effect';
import { Primitive } from '@radix-ui/react-primitive';

import type { Scope } from '@radix-ui/react-context';

/* -------------------------------------------------------------------------------------------------
* Avatar
* -----------------------------------------------------------------------------------------------*/
Expand All @@ -26,6 +25,7 @@ const [AvatarProvider, useAvatarContext] = createAvatarContext<AvatarContextValu

type AvatarElement = React.ElementRef<typeof Primitive.span>;
type PrimitiveSpanProps = React.ComponentPropsWithoutRef<typeof Primitive.span>;

interface AvatarProps extends PrimitiveSpanProps {}

const Avatar = React.forwardRef<AvatarElement, AvatarProps>(
Expand Down Expand Up @@ -54,25 +54,55 @@ const IMAGE_NAME = 'AvatarImage';

type AvatarImageElement = React.ElementRef<typeof Primitive.img>;
type PrimitiveImageProps = React.ComponentPropsWithoutRef<typeof Primitive.img>;

interface AvatarImageProps extends PrimitiveImageProps {
onLoadingStatusChange?: (status: ImageLoadingStatus) => void;
}

const AvatarImage = React.forwardRef<AvatarImageElement, AvatarImageProps>(
(props: ScopedProps<AvatarImageProps>, forwardedRef) => {
const { __scopeAvatar, src, onLoadingStatusChange = () => {}, ...imageProps } = props;

const context = useAvatarContext(IMAGE_NAME, __scopeAvatar);
const imageLoadingStatus = useImageLoadingStatus(src, imageProps.referrerPolicy);
const imageLoadingStatus = useImageLoadingStatus(src, props.asChild, imageProps.referrerPolicy);
const handleLoadingStatusChange = useCallbackRef((status: ImageLoadingStatus) => {
onLoadingStatusChange(status);
context.onImageLoadingStatusChange(status);
});

useLayoutEffect(() => {
if (imageLoadingStatus !== 'idle') {
handleLoadingStatusChange(imageLoadingStatus);
if (!src) {
handleLoadingStatusChange('error');
return;
}
}, [imageLoadingStatus, handleLoadingStatusChange]);

handleLoadingStatusChange('loading');
}, [handleLoadingStatusChange, src]);

if (props.asChild && props.children) {
// Ensure children is a valid React element
const child = React.Children.only(props.children) as React.ReactElement;

const { asChild, children, ...restProps } = props;

const childProps = child.props;

// Clone the child to add onLoad and onError event listeners
return React.cloneElement(child, {
...restProps,
...child.props,
onError: (event: React.SyntheticEvent<HTMLImageElement, Event>) => {
console.log('error');
handleLoadingStatusChange('error');
if (childProps.onError) childProps.onError(event);

Choose a reason for hiding this comment

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

question: @chaance do we also need to trigger onError callback from props?

Copy link
Author

Choose a reason for hiding this comment

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

For me make sense, considering that the child component might have some implementation of it 🤔.

But Chance can give us a better overview if it's necessary or not

Choose a reason for hiding this comment

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

IMO, to mimic the natural CSS flow, we should check and send the event to both the child's and parent's callback, so that if any using the parent callback the code won't break thus avoiding chaos.

},
onLoad: (event: React.SyntheticEvent<HTMLImageElement, Event>) => {
console.log('loaded');
handleLoadingStatusChange('loaded');
if (childProps.onLoad) childProps.onLoad(event);
},
});
}

return imageLoadingStatus === 'loaded' ? (
<Primitive.img {...imageProps} ref={forwardedRef} src={src} />
Expand All @@ -89,6 +119,7 @@ AvatarImage.displayName = IMAGE_NAME;
const FALLBACK_NAME = 'AvatarFallback';

type AvatarFallbackElement = React.ElementRef<typeof Primitive.span>;

interface AvatarFallbackProps extends PrimitiveSpanProps {
delayMs?: number;
}
Expand Down Expand Up @@ -116,10 +147,14 @@ AvatarFallback.displayName = FALLBACK_NAME;

/* -----------------------------------------------------------------------------------------------*/

function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttributeReferrerPolicy) {
function useImageLoadingStatus(src?: string, bypass?: boolean, referrerPolicy?: React.HTMLAttributeReferrerPolicy) {
const [loadingStatus, setLoadingStatus] = React.useState<ImageLoadingStatus>('idle');

useLayoutEffect(() => {
if (bypass) {
setLoadingStatus('idle');
return;
}
if (!src) {
setLoadingStatus('error');
return;
Expand All @@ -144,10 +179,11 @@ function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttribut
return () => {
isMounted = false;
};
}, [src, referrerPolicy]);
}, [src, bypass, referrerPolicy]);

return loadingStatus;
}

const Root = Avatar;
const Image = AvatarImage;
const Fallback = AvatarFallback;
Expand Down