Avatar and AvatarGroup with color support for fallback#793
Avatar and AvatarGroup with color support for fallback#793kodiakhq[bot] merged 2 commits intorad-ui:mainfrom
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
docs/app/docs/components/avatar/docs/codeUsage.jsOops! Something went wrong! :( ESLint: 8.56.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/docs/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces a Changes
Sequence DiagramsequenceDiagram
participant Avatar
participant AvatarPrimitiveFallback
Avatar->>AvatarPrimitiveFallback: Pass color prop
AvatarPrimitiveFallback-->>Avatar: Render with data-accent-color
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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
CodeRabbit Configuration File (
|
![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 (7)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (1)
12-12: Fix spacing in function parameters.There's an extra space before the closing curly brace in the destructured parameters.
-const AvatarPrimitiveFallback = ({ children, className = '' , color = ''}: AvatarPrimitiveFallback +const AvatarPrimitiveFallback = ({ children, className = '', color = '' }: AvatarPrimitiveFallback🧰 Tools
🪛 eslint
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Avatar/tests/Avatar.test.tsx (1)
24-27: Remove unnecessary async keyword.The test doesn't contain any asynchronous operations, so the
asynckeyword can be removed.- test('renders color for fallback when src is not provided', async() => { + test('renders color for fallback when src is not provided', () => {Otherwise, the test case looks good and properly verifies the color prop functionality.
src/components/ui/Avatar/Avatar.tsx (1)
14-14: Fix TypeScript style: Add space after colon.Add a space after the colon in the type definition for consistency.
- color?:string, + color?: string,src/components/ui/Avatar/stories/Avatar.stories.js (1)
45-50: Add newline at end of file.Add a newline at the end of the file to satisfy ESLint requirements.
} -}; +}; +The story implementation looks good and provides a clear example of the color prop usage.
🧰 Tools
🪛 eslint
[error] 50-50: Newline required at end of file but not found.
(eol-last)
src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js (1)
37-46: Fix object literal spacing and add color variants.The story demonstrates color support well, but has some formatting issues.
Apply these fixes:
- { src: '', fallback: 'RU'}, + { src: '', fallback: 'RU' }, - { src: '', fallback: 'PK'}, + { src: '', fallback: 'PK' },Consider adding more color variants to demonstrate the full range of supported colors.
🧰 Tools
🪛 eslint
[error] 40-40: A space is required before '}'.
(object-curly-spacing)
[error] 41-41: A space is required before '}'.
(object-curly-spacing)
src/components/ui/AvatarGroup/AvatarGroup.tsx (1)
17-17: Consider using a union type for color prop.Instead of using a generic string type, consider defining specific color options for better type safety and documentation.
- color?: string; + color?: 'blue' | 'green' | 'red' | 'yellow' | 'purple'; // add supported colorssrc/components/ui/AvatarGroup/tests/AvatarGroup.test.js (1)
72-76: Enhance color testing coverage.While the current test verifies basic color functionality, consider adding these test cases:
- Test with different color values
- Test color inheritance from AvatarGroup to individual Avatars
- Test behavior when color prop is undefined
Example addition:
test('inherits color prop correctly to all avatars', () => { const colors = ['blue', 'green', 'red']; colors.forEach(color => { render(<AvatarGroup avatars={avatarsWithFallback} color={color} />); expect(screen.getByText('A')).toHaveAttribute('data-accent-color', color); expect(screen.getByText('B')).toHaveAttribute('data-accent-color', color); cleanup(); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/ui/Avatar/Avatar.tsx(2 hunks)src/components/ui/Avatar/stories/Avatar.stories.js(1 hunks)src/components/ui/Avatar/tests/Avatar.test.tsx(1 hunks)src/components/ui/AvatarGroup/AvatarGroup.tsx(1 hunks)src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js(1 hunks)src/components/ui/AvatarGroup/tests/AvatarGroup.test.js(1 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js
[error] 30-30: A space is required before '}'.
(object-curly-spacing)
[error] 31-31: A space is required before '}'.
(object-curly-spacing)
[error] 40-40: A space is required before '}'.
(object-curly-spacing)
[error] 41-41: A space is required before '}'.
(object-curly-spacing)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx
[error] 12-12: A space is required before '}'.
(object-curly-spacing)
src/components/ui/Avatar/stories/Avatar.stories.js
[error] 50-50: Newline required at end of file but not found.
(eol-last)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (1)
9-9: LGTM! Clean implementation of the color prop.The implementation follows React best practices by:
- Using an optional prop with proper TypeScript typing
- Using a data attribute for styling customization
Also applies to: 19-19
src/components/ui/Avatar/Avatar.tsx (1)
18-18: LGTM! Clean prop forwarding implementation.The color prop is correctly forwarded to the Fallback component while maintaining the component's structure.
Also applies to: 27-29
src/components/ui/AvatarGroup/stories/AvatarGroup.stories.js (1)
27-35: LGTM! Good test coverage for fallback behavior.The story effectively demonstrates the fallback behavior when src is empty.
🧰 Tools
🪛 eslint
[error] 30-30: A space is required before '}'.
(object-curly-spacing)
[error] 31-31: A space is required before '}'.
(object-curly-spacing)
src/components/ui/AvatarGroup/AvatarGroup.tsx (1)
21-21: LGTM! Color prop is correctly passed to fallback component.The implementation properly handles the color prop flow from AvatarGroup to AvatarPrimitiveFallback.
Also applies to: 26-26
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/app/docs/components/avatar/docs/codeUsage.js (1)
9-9: Consider documenting color values and CSS variable relationship.While the example demonstrates the new
colorprop usage, it would be helpful to document:
- The list of supported color values
- How these values map to CSS variables in the fallback implementation
- <Avatar fallback="AA" color='pink'/> + {/* Colors map to CSS variables (--rad-ui-color-{color}-500/900) */} + <Avatar fallback="AA" color="pink" />docs/app/docs/components/avatar/page.js (1)
21-21: Enhance color prop documentation.The description could be more specific about:
- The supported color values
- Whether custom colors are supported
- The actual default value (string 'null' vs JavaScript null)
- {prop: 'color', type: 'string', default: 'null', description: 'Accent Color of the text initials or placeholder displayed when the image fails to load or if no src is provided.', id: 'color'}, + {prop: 'color', type: 'string', default: null, description: 'Accent color for the fallback. Supports theme colors like "pink", "blue", etc. Custom colors are not supported.', id: 'color'},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/app/docs/components/avatar/docs/codeUsage.js(1 hunks)docs/app/docs/components/avatar/page.js(2 hunks)
🔇 Additional comments (1)
docs/app/docs/components/avatar/page.js (1)
31-31: LGTM! Example demonstrates the new color prop effectively.The example in the hero section provides a clear visual demonstration of the color prop usage.
|
Merging, note that the docs changes wont update til we do another npm release! From next PRs lets do core library updates in one PR, and docs updates after an NPM release! |
kotAPI
added
the
automerge
2 participants
Added the color prop for fallback with tests and storybook example. Updated the docs too will work once we do a npm release.
Summary by CodeRabbit
Release Notes
New Features
colorproperty to Avatar and AvatarGroup components.Improvements
Testing