From 8d62e7eb80de6dec43e080f7d98ed5bd7cf719fe Mon Sep 17 00:00:00 2001 From: rubenskj Date: Thu, 4 Jul 2024 14:21:06 -0300 Subject: [PATCH 1/4] Adding support for nextjs image component. --- packages/react/avatar/src/Avatar.stories.tsx | 67 +++++++++++++++++ packages/react/avatar/src/Avatar.test.tsx | 21 +----- packages/react/avatar/src/Avatar.tsx | 75 ++++++++++---------- 3 files changed, 107 insertions(+), 56 deletions(-) diff --git a/packages/react/avatar/src/Avatar.stories.tsx b/packages/react/avatar/src/Avatar.stories.tsx index 0ee66a2bf..16b032ff6 100644 --- a/packages/react/avatar/src/Avatar.stories.tsx +++ b/packages/react/avatar/src/Avatar.stories.tsx @@ -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 ( +
+ framework test +
+ ); +}; + export const Styled = () => ( <>

Without image & with fallback

@@ -33,6 +44,34 @@ export const Styled = () => ( + +

With image framework component

+ + + + + + + +

With image framework component & with fallback (but broken src)

+ + + + + + ); @@ -58,6 +97,34 @@ export const Chromatic = () => ( + +

With image framework component

+ + + + + + + +

With image framework component & with fallback (but broken src)

+ + + + + + ); Chromatic.parameters = { chromatic: { disable: false, delay: 1000 } }; diff --git a/packages/react/avatar/src/Avatar.test.tsx b/packages/react/avatar/src/Avatar.test.tsx index e1705c6f3..3c86efc75 100644 --- a/packages/react/avatar/src/Avatar.test.tsx +++ b/packages/react/avatar/src/Avatar.test.tsx @@ -1,6 +1,6 @@ import { axe } from 'jest-axe'; import type { RenderResult } from '@testing-library/react'; -import { render } from '@testing-library/react'; +import { fireEvent, render } from '@testing-library/react'; import * as Avatar from '@radix-ui/react-avatar'; const ROOT_TEST_ID = 'avatar-root'; @@ -27,24 +27,6 @@ describe('given an Avatar with fallback and no image', () => { describe('given an Avatar with fallback and a working 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( @@ -66,6 +48,7 @@ describe('given an Avatar with fallback and a working image', () => { }); it('should render the image after it has loaded', async () => { + fireEvent.load(rendered.getByRole('img', { hidden: true })); image = await rendered.findByRole('img'); expect(image).toBeInTheDocument(); }); diff --git a/packages/react/avatar/src/Avatar.tsx b/packages/react/avatar/src/Avatar.tsx index 3749fc218..5477a059e 100644 --- a/packages/react/avatar/src/Avatar.tsx +++ b/packages/react/avatar/src/Avatar.tsx @@ -56,27 +56,57 @@ type AvatarImageElement = React.ElementRef; type PrimitiveImageProps = React.ComponentPropsWithoutRef; interface AvatarImageProps extends PrimitiveImageProps { onLoadingStatusChange?: (status: ImageLoadingStatus) => void; + component?: React.ComponentType; } const AvatarImage = React.forwardRef( (props: ScopedProps, forwardedRef) => { - const { __scopeAvatar, src, onLoadingStatusChange = () => {}, ...imageProps } = props; + const { + __scopeAvatar, + src, + onLoadingStatusChange = () => {}, + component: Component = Primitive.img, + ...imageProps + } = props; + const context = useAvatarContext(IMAGE_NAME, __scopeAvatar); - const imageLoadingStatus = useImageLoadingStatus(src); const handleLoadingStatusChange = useCallbackRef((status: ImageLoadingStatus) => { onLoadingStatusChange(status); context.onImageLoadingStatusChange(status); }); useLayoutEffect(() => { - if (imageLoadingStatus !== 'idle') { - handleLoadingStatusChange(imageLoadingStatus); + if (!src) { + handleLoadingStatusChange('error'); + return; } - }, [imageLoadingStatus, handleLoadingStatusChange]); - return imageLoadingStatus === 'loaded' ? ( - - ) : null; + handleLoadingStatusChange('loading'); + }, [handleLoadingStatusChange, src]); + + return ( + { + handleLoadingStatusChange('loaded'); + if (imageProps.onLoad) { + imageProps.onLoad(event); + } + }} + onError={(event) => { + handleLoadingStatusChange('error'); + if (imageProps.onError) { + imageProps.onError(event); + } + }} + style={{ + display: context.imageLoadingStatus !== 'loaded' ? 'none' : undefined, + ...imageProps.style, + }} + /> + ); } ); @@ -116,35 +146,6 @@ AvatarFallback.displayName = FALLBACK_NAME; /* -----------------------------------------------------------------------------------------------*/ -function useImageLoadingStatus(src?: string) { - const [loadingStatus, setLoadingStatus] = React.useState('idle'); - - useLayoutEffect(() => { - if (!src) { - setLoadingStatus('error'); - return; - } - - let isMounted = true; - const image = new window.Image(); - - const updateStatus = (status: ImageLoadingStatus) => () => { - if (!isMounted) return; - setLoadingStatus(status); - }; - - setLoadingStatus('loading'); - image.onload = updateStatus('loaded'); - image.onerror = updateStatus('error'); - image.src = src; - - return () => { - isMounted = false; - }; - }, [src]); - - return loadingStatus; -} const Root = Avatar; const Image = AvatarImage; const Fallback = AvatarFallback; From ba725a996e09d937a71ad49f7b92264f201b0e67 Mon Sep 17 00:00:00 2001 From: rubenskj Date: Thu, 4 Jul 2024 14:23:19 -0300 Subject: [PATCH 2/4] Adding release file --- .yarn/versions/48669fbc.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .yarn/versions/48669fbc.yml diff --git a/.yarn/versions/48669fbc.yml b/.yarn/versions/48669fbc.yml new file mode 100644 index 000000000..215ab005a --- /dev/null +++ b/.yarn/versions/48669fbc.yml @@ -0,0 +1,2 @@ +releases: + "@radix-ui/react-avatar": minor From a72e70344d0a659781f150303dbab1da1a70148a Mon Sep 17 00:00:00 2001 From: Udhayarajan <77388817+Udhayarajan@users.noreply.github.com> Date: Sat, 14 Sep 2024 01:54:40 +0530 Subject: [PATCH 3/4] Feature/adding avatar component (#1) * fix: reverted the ComponentType approach and re-added internal optimization, made use of `children` and `asChild` props as boolean to determine the usage of `Primitive.img` or the child element * chore: version updated --- .yarn/versions/48669fbc.yml | 1 + packages/react/avatar/src/Avatar.stories.tsx | 37 +++----- packages/react/avatar/src/Avatar.test.tsx | 84 ++++++++++++++++- packages/react/avatar/src/Avatar.tsx | 99 +++++++++++++------- 4 files changed, 163 insertions(+), 58 deletions(-) diff --git a/.yarn/versions/48669fbc.yml b/.yarn/versions/48669fbc.yml index 215ab005a..834dea45c 100644 --- a/.yarn/versions/48669fbc.yml +++ b/.yarn/versions/48669fbc.yml @@ -1,2 +1,3 @@ releases: "@radix-ui/react-avatar": minor + primitives: patch diff --git a/packages/react/avatar/src/Avatar.stories.tsx b/packages/react/avatar/src/Avatar.stories.tsx index 16b032ff6..00e0bd265 100644 --- a/packages/react/avatar/src/Avatar.stories.tsx +++ b/packages/react/avatar/src/Avatar.stories.tsx @@ -50,10 +50,11 @@ export const Styled = () => ( + asChild + > + + @@ -61,13 +62,9 @@ export const Styled = () => (

With image framework component & with fallback (but broken src)

- + + + @@ -100,13 +97,9 @@ export const Chromatic = () => (

With image framework component

- + + + @@ -114,13 +107,9 @@ export const Chromatic = () => (

With image framework component & with fallback (but broken src)

- + + + diff --git a/packages/react/avatar/src/Avatar.test.tsx b/packages/react/avatar/src/Avatar.test.tsx index 3c86efc75..7d938d684 100644 --- a/packages/react/avatar/src/Avatar.test.tsx +++ b/packages/react/avatar/src/Avatar.test.tsx @@ -1,12 +1,14 @@ import { axe } from 'jest-axe'; import type { RenderResult } from '@testing-library/react'; -import { fireEvent, render } 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; @@ -27,6 +29,25 @@ describe('given an Avatar with fallback and no image', () => { describe('given an Avatar with fallback and a working 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( @@ -48,7 +69,6 @@ describe('given an Avatar with fallback and a working image', () => { }); it('should render the image after it has loaded', async () => { - fireEvent.load(rendered.getByRole('img', { hidden: true })); image = await rendered.findByRole('img'); expect(image).toBeInTheDocument(); }); @@ -83,3 +103,63 @@ describe('given an Avatar with fallback and delayed render', () => { expect(fallback).toBeInTheDocument(); }); }); + +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( + + + {FRAMEWORK_IMAGE_ALT_TEXT} + + {FALLBACK_TEXT} + + ); + 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(); + }); +}); diff --git a/packages/react/avatar/src/Avatar.tsx b/packages/react/avatar/src/Avatar.tsx index 5477a059e..f83ce60d6 100644 --- a/packages/react/avatar/src/Avatar.tsx +++ b/packages/react/avatar/src/Avatar.tsx @@ -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 * -----------------------------------------------------------------------------------------------*/ @@ -26,6 +25,7 @@ const [AvatarProvider, useAvatarContext] = createAvatarContext; type PrimitiveSpanProps = React.ComponentPropsWithoutRef; + interface AvatarProps extends PrimitiveSpanProps {} const Avatar = React.forwardRef( @@ -54,22 +54,17 @@ const IMAGE_NAME = 'AvatarImage'; type AvatarImageElement = React.ElementRef; type PrimitiveImageProps = React.ComponentPropsWithoutRef; + interface AvatarImageProps extends PrimitiveImageProps { onLoadingStatusChange?: (status: ImageLoadingStatus) => void; - component?: React.ComponentType; } const AvatarImage = React.forwardRef( (props: ScopedProps, forwardedRef) => { - const { - __scopeAvatar, - src, - onLoadingStatusChange = () => {}, - component: Component = Primitive.img, - ...imageProps - } = props; + const { __scopeAvatar, src, onLoadingStatusChange = () => {}, ...imageProps } = props; const context = useAvatarContext(IMAGE_NAME, __scopeAvatar); + const imageLoadingStatus = useImageLoadingStatus(src, props.asChild); const handleLoadingStatusChange = useCallbackRef((status: ImageLoadingStatus) => { onLoadingStatusChange(status); context.onImageLoadingStatusChange(status); @@ -84,29 +79,34 @@ const AvatarImage = React.forwardRef( handleLoadingStatusChange('loading'); }, [handleLoadingStatusChange, src]); - return ( - { - handleLoadingStatusChange('loaded'); - if (imageProps.onLoad) { - imageProps.onLoad(event); - } - }} - onError={(event) => { + 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) => { + console.log('error'); handleLoadingStatusChange('error'); - if (imageProps.onError) { - imageProps.onError(event); - } - }} - style={{ - display: context.imageLoadingStatus !== 'loaded' ? 'none' : undefined, - ...imageProps.style, - }} - /> - ); + if (childProps.onError) childProps.onError(event); + }, + onLoad: (event: React.SyntheticEvent) => { + console.log('loaded'); + handleLoadingStatusChange('loaded'); + if (childProps.onLoad) childProps.onLoad(event); + }, + }); + } + + return imageLoadingStatus === 'loaded' ? ( + + ) : null; } ); @@ -119,6 +119,7 @@ AvatarImage.displayName = IMAGE_NAME; const FALLBACK_NAME = 'AvatarFallback'; type AvatarFallbackElement = React.ElementRef; + interface AvatarFallbackProps extends PrimitiveSpanProps { delayMs?: number; } @@ -146,6 +147,40 @@ AvatarFallback.displayName = FALLBACK_NAME; /* -----------------------------------------------------------------------------------------------*/ +function useImageLoadingStatus(src?: string, bypass?: boolean) { + const [loadingStatus, setLoadingStatus] = React.useState('idle'); + + useLayoutEffect(() => { + if (bypass) { + setLoadingStatus('idle'); + return; + } + if (!src) { + setLoadingStatus('error'); + return; + } + + let isMounted = true; + const image = new window.Image(); + + const updateStatus = (status: ImageLoadingStatus) => () => { + if (!isMounted) return; + setLoadingStatus(status); + }; + + setLoadingStatus('loading'); + image.onload = updateStatus('loaded'); + image.onerror = updateStatus('error'); + image.src = src; + + return () => { + isMounted = false; + }; + }, [src, bypass]); + + return loadingStatus; +} + const Root = Avatar; const Image = AvatarImage; const Fallback = AvatarFallback; From fed51e6b904c408212b30806f39f322838d76705 Mon Sep 17 00:00:00 2001 From: Udhayarajan <77388817+Udhayarajan@users.noreply.github.com> Date: Sat, 14 Sep 2024 21:22:17 +0530 Subject: [PATCH 4/4] Feature/adding avatar component (#2) * test: inconsistency in stories with `asChild` props * fix: returns null dynamically if fails to fetch image from FrameworkImage's Component * test: reverted accidental removal of `waitFor` --- packages/react/avatar/src/Avatar.stories.tsx | 21 ++++++++++-- packages/react/avatar/src/Avatar.test.tsx | 2 +- packages/react/avatar/src/Avatar.tsx | 35 ++++++++++++-------- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/packages/react/avatar/src/Avatar.stories.tsx b/packages/react/avatar/src/Avatar.stories.tsx index 00e0bd265..c1c5d0fc4 100644 --- a/packages/react/avatar/src/Avatar.stories.tsx +++ b/packages/react/avatar/src/Avatar.stories.tsx @@ -62,7 +62,12 @@ export const Styled = () => (

With image framework component & with fallback (but broken src)

- + @@ -97,7 +102,12 @@ export const Chromatic = () => (

With image framework component

- + @@ -107,7 +117,12 @@ export const Chromatic = () => (

With image framework component & with fallback (but broken src)

- + diff --git a/packages/react/avatar/src/Avatar.test.tsx b/packages/react/avatar/src/Avatar.test.tsx index df996b02b..1ac65aead 100644 --- a/packages/react/avatar/src/Avatar.test.tsx +++ b/packages/react/avatar/src/Avatar.test.tsx @@ -1,6 +1,6 @@ import { axe } from 'jest-axe'; import type { RenderResult } from '@testing-library/react'; -import { render } from '@testing-library/react'; +import { render, waitFor } from '@testing-library/react'; import * as Avatar from '@radix-ui/react-avatar'; const ROOT_TEST_ID = 'avatar-root'; diff --git a/packages/react/avatar/src/Avatar.tsx b/packages/react/avatar/src/Avatar.tsx index 0ae2cc976..2fe160388 100644 --- a/packages/react/avatar/src/Avatar.tsx +++ b/packages/react/avatar/src/Avatar.tsx @@ -1,10 +1,11 @@ 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 * -----------------------------------------------------------------------------------------------*/ @@ -25,7 +26,6 @@ const [AvatarProvider, useAvatarContext] = createAvatarContext; type PrimitiveSpanProps = React.ComponentPropsWithoutRef; - interface AvatarProps extends PrimitiveSpanProps {} const Avatar = React.forwardRef( @@ -54,7 +54,6 @@ const IMAGE_NAME = 'AvatarImage'; type AvatarImageElement = React.ElementRef; type PrimitiveImageProps = React.ComponentPropsWithoutRef; - interface AvatarImageProps extends PrimitiveImageProps { onLoadingStatusChange?: (status: ImageLoadingStatus) => void; } @@ -62,24 +61,29 @@ interface AvatarImageProps extends PrimitiveImageProps { const AvatarImage = React.forwardRef( (props: ScopedProps, forwardedRef) => { const { __scopeAvatar, src, onLoadingStatusChange = () => {}, ...imageProps } = props; - const context = useAvatarContext(IMAGE_NAME, __scopeAvatar); - const imageLoadingStatus = useImageLoadingStatus(src, props.asChild, imageProps.referrerPolicy); + const { loadingStatus: imageLoadingStatus, setLoadingStatus } = useImageLoadingStatus( + src, + props.asChild, + imageProps.referrerPolicy + ); const handleLoadingStatusChange = useCallbackRef((status: ImageLoadingStatus) => { + setLoadingStatus(status); onLoadingStatusChange(status); context.onImageLoadingStatusChange(status); }); useLayoutEffect(() => { - if (!src) { - handleLoadingStatusChange('error'); - return; + if (imageLoadingStatus !== 'idle') { + handleLoadingStatusChange(imageLoadingStatus); } - - handleLoadingStatusChange('loading'); - }, [handleLoadingStatusChange, src]); + }, [imageLoadingStatus, handleLoadingStatusChange]); if (props.asChild && props.children) { + if (imageLoadingStatus === 'error') { + return null; + } + // Ensure children is a valid React element const child = React.Children.only(props.children) as React.ReactElement; @@ -147,7 +151,11 @@ AvatarFallback.displayName = FALLBACK_NAME; /* -----------------------------------------------------------------------------------------------*/ -function useImageLoadingStatus(src?: string, bypass?: boolean, referrerPolicy?: React.HTMLAttributeReferrerPolicy) { +function useImageLoadingStatus( + src?: string, + bypass?: boolean, + referrerPolicy?: React.HTMLAttributeReferrerPolicy +) { const [loadingStatus, setLoadingStatus] = React.useState('idle'); useLayoutEffect(() => { @@ -181,9 +189,8 @@ function useImageLoadingStatus(src?: string, bypass?: boolean, referrerPolicy?: }; }, [src, bypass, referrerPolicy]); - return loadingStatus; + return { loadingStatus, setLoadingStatus }; } - const Root = Avatar; const Image = AvatarImage; const Fallback = AvatarFallback;