feat(image): add loading src to display loading state #4783
Conversation
|
|
Someone is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update introduces a new React component and supporting files to differentiate between the loading and error states of an image. A new property, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant IC as Image Component
participant UH as useImage Hook
participant N as Network
U->>IC: Request Image (with loadingSrc & fallbackSrc)
IC->>UH: Pass props for processing
UH->>N: Attempt to load main image
alt Image is still loading
UH-->>IC: Return loading state (display loadingSrc)
else Image loads successfully
UH-->>IC: Return main image for display
else Image fails to load
UH-->>IC: Return error state (display fallbackSrc)
end
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/docs/content/components/image/customLoading.raw.jsx (1)
8-9: Consider using local assets for demo reliability.The demo uses external services for both the placeholder and the delayed image:
via.placeholder.comfor loading staterequestly.iofor simulating delayThis could make the demo unreliable if these services are down.
Consider using local assets hosted within the project for both the placeholder and the main image. For delay simulation, consider using a local development proxy or built-in delay mechanism.
packages/components/image/stories/image.stories.tsx (2)
125-125: Consider more reliable delay simulation method.Multiple stories use
requestly.iofor delay simulation which could make the stories unreliable in offline environments or if the service is down.Consider using React Suspense or a local delay mechanism for more reliable story demonstrations.
Also applies to: 146-147
165-177: Add story description for CustomLoadingAndFallback.The new story demonstrates an important use case but lacks a description of what it's showcasing.
Add a JSDoc comment explaining the purpose of this story and when to use both loadingSrc and fallbackSrc together.
packages/components/image/src/use-image.ts (1)
27-30: Enhance loadingSrc prop documentation.The current documentation is minimal. Consider adding:
- Usage example
- Default value
- Note about interaction with disableSkeleton
/** * A loading image. + * @example + * ```jsx + * <Image loadingSrc="/loading.gif" src="/main.jpg" /> + * ``` + * @default undefined + * @note When provided, disables the default skeleton loader */apps/docs/content/docs/components/image.mdx (1)
165-169: API Enhancement:loadingSrcProp Entry
The addition of theloadingSrcprop within the API table is well-documented. Its type, description, and default value are clearly stated, providing comprehensive guidance for developers on how to use this new feature. Consider verifying whether the default value ("-") accurately represents the intended default state (e.g., an empty string might be more conventional) if applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/docs/content/components/image/customLoading.raw.jsx(1 hunks)apps/docs/content/components/image/customLoading.ts(1 hunks)apps/docs/content/docs/components/image.mdx(2 hunks)packages/components/image/src/image.tsx(2 hunks)packages/components/image/src/use-image.ts(7 hunks)packages/components/image/stories/image.stories.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/docs/content/components/image/customLoading.ts
🔇 Additional comments (4)
packages/components/image/src/image.tsx (1)
16-16: LGTM! Clean integration of loadingSrc.The loadingSrc prop is well-integrated into the existing component structure, maintaining consistency with the current implementation pattern.
Also applies to: 49-49
packages/components/image/src/use-image.ts (1)
140-142: LGTM! Clear separation of loading states.The loading state logic is well-organized with clear separation between:
- Loading state with custom image
- Error state with fallback
- Default skeleton loader
apps/docs/content/docs/components/image.mdx (2)
66-73: Documentation Clarity: "Custom Loading Image" Section
The new section clearly explains the purpose of theloadingSrcprop and how it allows a custom loading image to be displayed while the main image is still loading. The use of a dedicated code demo and a note about the simulated network delay is consistent with other sections in the document.
172-176: Improved Clarity:fallbackSrcProp Description Update
The updated description for thefallbackSrcprop now specifies that it is used when the main image fails to load or encounters an error, which is clearer than the previous explanation. This update helps avoid ambiguity about the prop’s behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/docs/content/docs/components/image.mdx (1)
57-60: Add "the" before "Image component" for better readability.The sentence should read "The Image component has a built-in
skeletonanimation..."-Image component has a built-in `skeleton` animation to indicate the image is loading in case the `loadingSrc` is not defined. +The Image component has a built-in `skeleton` animation to indicate the image is loading in case the `loadingSrc` is not defined.🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: You might be missing the article “the” here.
Context: ...d} /> ### Animated (Skeleton) Loading Image component has a built-inskeletonani...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/docs/content/components/image/customLoading.raw.jsx(1 hunks)apps/docs/content/components/image/fallback.raw.jsx(1 hunks)apps/docs/content/components/image/index.ts(2 hunks)apps/docs/content/docs/components/image.mdx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/content/components/image/customLoading.raw.jsx
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/image.mdx
[uncategorized] ~58-~58: You might be missing the article “the” here.
Context: ...d} /> ### Animated (Skeleton) Loading Image component has a built-in skeleton ani...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (5)
apps/docs/content/components/image/fallback.raw.jsx (1)
9-9: LGTM! Good example for demonstrating fallback behavior.The intentionally invalid image source effectively demonstrates the fallback functionality.
apps/docs/content/components/image/index.ts (1)
5-5: LGTM! Clean integration of the new custom loading example.The changes follow the existing pattern for importing and exporting examples.
Also applies to: 14-14
apps/docs/content/docs/components/image.mdx (3)
65-71: LGTM! Clear documentation for the new custom loading feature.The new section effectively explains the
loadingSrcprop with a practical example and network simulation note.
83-84: LGTM! Helpful note about combining loading and fallback states.Good addition clarifying that both states can be handled simultaneously.
164-169: LGTM! Well-documented API properties.The new
loadingSrcprop and updatedfallbackSrcdescription are clear and accurate.Also applies to: 173-173
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/image/__tests__/image.test.tsx (3)
30-39: Enhance loading state test coverage.The test could be improved in several ways:
- Add cleanup to prevent test pollution
- Add explicit null check instead of using non-null assertion
- Test the transition from loading to loaded state
Consider this improved version:
test("renders loading source while loading the image.", async () => { const wrapper = render(<Image loadingSrc={loadingSrc} src={src} />); const imageParent = wrapper.getByRole("img").parentElement; - expect(imageParent?.getAttribute("data-testid")).toEqual("heroUI/image_parent"); + expect(imageParent).not.toBeNull(); + expect(imageParent!.getAttribute("data-testid")).toEqual("heroUI/image_parent"); const computedStyle = window.getComputedStyle(imageParent!); expect(computedStyle.backgroundImage).toBe(`url(${loadingSrc})`); + + // Test transition to loaded state + fireEvent.load(wrapper.getByRole("img")); + await waitFor(() => { + const updatedStyle = window.getComputedStyle(imageParent!); + expect(updatedStyle.backgroundImage).toBe(`url(${src})`); + }); + + // Cleanup + wrapper.unmount(); });
41-55: Optimize error handling test.The test has some potential improvements:
- The 6-second timeout seems excessive for a unit test
- Missing cleanup
- Could use more specific error simulation
Consider this improved version:
test("renders fallback source if src is wrong or not found.", async () => { const onError = jest.fn(); const wrapper = render( <Image alt="test" fallbackSrc={fallbackSrc} src="wrong-src-address" onError={onError} />, ); const imageParent = wrapper.getByRole("img").parentElement; - fireEvent.error(wrapper.getByRole("img")); + // Simulate specific error type + fireEvent.error(wrapper.getByRole("img"), { + target: { error: new Error('Failed to load image') } + }); - await waitFor(() => expect(onError).toHaveBeenCalled(), {timeout: 5_000}); + await waitFor(() => expect(onError).toHaveBeenCalled(), {timeout: 1000}); expect(imageParent?.getAttribute("data-testid")).toEqual("heroUI/image_parent"); const computedStyle = window.getComputedStyle(imageParent!); expect(computedStyle.backgroundImage).toBe(`url(${fallbackSrc})`); -}, 6_000); + + // Cleanup + wrapper.unmount(); +}, 2000);
57-61: Add cleanup to prevent test pollution.The test is well-structured but should include cleanup to prevent potential test pollution.
test("renders image if there is no loading or fallback behavior defined", async () => { const wrapper = render(<Image src={src} />); expect(wrapper.getByRole("img")).toHaveAttribute("src", src); + wrapper.unmount(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/image/__tests__/image.test.tsx(2 hunks)packages/components/image/src/image.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/image/src/image.tsx
🔇 Additional comments (1)
packages/components/image/__tests__/image.test.tsx (1)
8-8: Consider using an absolute URL for loadingSrc.Using a relative path
/images/local-image-small.jpgmight cause issues in different test environments. Consider using an absolute URL (like the placeholder URLs used forsrcandfallbackSrc) to ensure consistent test behavior across environments.
I couldn't. The second test needs the |
|
you may take a look at |
done. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/image/__tests__/image.test.tsx(4 hunks)
🔇 Additional comments (3)
packages/components/image/__tests__/image.test.tsx (3)
50-71: LGTM! Well-structured test for loading state.The test thoroughly verifies both the loading state and the final loaded state, including:
- Loading image display
- onLoad callback
- Proper cleanup
110-115: LGTM! Clear test description update.The test description now accurately reflects the expanded scope including loading behavior.
33-33: LGTM! Consistent cleanup practice.Good addition of wrapper.unmount() calls across all test cases to prevent memory leaks and test interference.
Also applies to: 47-47, 70-70, 107-107, 114-114, 121-121, 129-129, 143-143, 164-164
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/image/__tests__/image.test.tsx (2)
30-38: Optimize cleanup code performance.The cleanup code is good but can be optimized by avoiding the delete operator.
afterAll(() => { - delete window.Image.prototype._onload; + window.Image.prototype._onload = undefined; Object.defineProperty(window.Image.prototype, "onload", { value: null, writable: true, configurable: true, }); });🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
84-130: Enhance error handling test.The test case is good but can be improved in two areas:
- Performance optimization for cleanup
- More robust error simulation
const cleanup = () => { - delete window.Image.prototype._onerror; + window.Image.prototype._onerror = undefined; Object.defineProperty(window.Image.prototype, "onerror", { value: null, writable: true, configurable: true, }); }; act(() => { - imageOnerror(); + imageOnerror(new Event('error')); });🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/image/__tests__/image.test.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/image/__tests__/image.test.tsx
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 103-103: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
packages/components/image/__tests__/image.test.tsx (4)
7-8: LGTM! Constants are well-defined and descriptive.The constants are appropriately named and their values are clear and meaningful.
13-28: LGTM! Image onload tracking setup is well-implemented.The setup code properly tracks the image onload event by modifying the Image prototype, which is essential for testing loading states.
61-82: LGTM! Loading state test is comprehensive.The test case thoroughly verifies:
- Initial loading image display
- Proper loading state transition
- onLoad callback invocation
- Cleanup after test completion
132-137: LGTM! Test cases have proper cleanup.All test cases now include proper cleanup by unmounting components after test completion.
Also applies to: 139-187
Recreates the #3664 PR (fixes #3640).
Summary by CodeRabbit
New Features
Appcomponent to demonstrate custom loading images.Documentation