Fix Avatar + AvatarGroup composable API + other fixes #1080
Conversation
WalkthroughThe Avatar component was refactored from a monolithic, prop-driven component into a namespace exposing modular subcomponents: AvatarRoot, AvatarImage, and AvatarFallback. Supporting context and fragment components were introduced for modularity. Related tests, stories, and styling were updated to reflect the new composition pattern and class naming conventions. AvatarGroup and several primitive components were adjusted for compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AvatarRoot
participant AvatarImage
participant AvatarFallback
participant AvatarContext
User->>AvatarRoot: Render with props (size, variant, color, customRootClass)
AvatarRoot->>AvatarContext: Provide context (size, variant, color, rootClass)
AvatarRoot->>AvatarImage: Render as child (receives context)
AvatarRoot->>AvatarFallback: Render as child (receives context)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (9)
src/components/ui/AvatarGroup/tests/AvatarGroup.test.js (1)
72-76: Add explanatory comment for future maintainersConsider adding a brief explanatory comment above the commented-out test explaining why it was disabled (e.g., "The color prop was removed from AvatarGroup in PR #1080"). This would help future maintainers understand the context without having to dig through commit history.
styles/themes/components/avatar.scss (1)
3-4: Avatar dimensions increased to 36x36pxThe base size of the avatar has been increased from 32x32px to 36x36px, which will affect all instances of the component. Ensure this change is intentional and consistent with your design system.
src/core/primitives/Avatar/contexts/AvatarPrimitiveContext.tsx (1)
5-5: Missing semicolon in interface propertyThere's a missing semicolon after the
hasError: booleanproperty declaration, while other properties in the interface have semicolons.- hasError: boolean + hasError: boolean;src/core/customClassSwitcher/index.ts (1)
15-17: Standardized handling of customRootClass across componentsThe function now combines
customRootClasswith the kebab-cased component name, ensuring consistent class naming across the UI components. This change supports the Avatar component refactoring and standardizes the approach to custom class names throughout the codebase.However, I'd recommend adding documentation to clarify this behavior for developers:
/** * Applies a custom root class the user provides, else applies the default rad-ui classes to the component * Rad UI's classes are based on this logic + * @param customRootClass - Custom root class prefix that will be combined with the component name + * @param componentName - The name of the component to be kebab-cased and appended to the class + * @returns A string containing the full class name in the format `${customRootClass}-${kebabCasedComponentName}` or `rad-ui-${kebabCasedComponentName}` * */src/components/ui/Avatar/contexts/AvatarContext.tsx (1)
3-8: Consider using more specific types for better type safety.The
AvatarContextTypeinterface uses generic string types for all properties. For properties likesizeandvariantthat likely have a finite set of valid values, consider using string union types (e.g.,size?: 'sm' | 'md' | 'lg') to provide better type checking and IDE autocompletion.src/components/ui/Avatar/fragments/AvatarFallback.tsx (1)
5-5: Static analysis warning can be ignored.The static analysis tool incorrectly flags
AvatarPrimitiveFallbackPropsas unused, but it's being used in the component props type definition on line 9.🧰 Tools
🪛 GitHub Check: lint
[warning] 5-5:
'AvatarPrimitiveFallbackProps' is defined but never usedsrc/components/ui/Avatar/fragments/AvatarImage.tsx (1)
13-16: Remove unusedchildrenparameter from destructuring.The component destructures
childrenfrom props but doesn't use it anywhere. Since<img>elements don't support children in HTML, this parameter should be removed to avoid confusion.-const AvatarImage = ({ src = '', alt = '', children, ...props }: AvatarImageProps) => { +const AvatarImage = ({ src = '', alt = '', ...props }: AvatarImageProps) => { const { rootClass } = useContext(AvatarContext); return <AvatarPrimitiveImage className={clsx(`${rootClass}-image`)} src={src} alt={alt} {...props} />; };src/components/ui/Avatar/stories/Avatar.stories.tsx (1)
71-81: Good demonstration of Fallback-only usage.This story effectively demonstrates how to use the Avatar component without an image, which is an important use case that should be documented.
Consider adding a brief comment explaining when this pattern might be useful in a real application.
src/components/ui/Avatar/Avatar.tsx (1)
16-18: Clean implementation of compound component pattern.Exposing the fragment components through static properties on the main Avatar component creates a clean namespace and intuitive API.
One enhancement to consider is to define proper TypeScript types for the compound component to improve IDE autocompletion and type checking:
type AvatarComponent = React.FC & { Root: typeof AvatarRoot; Image: typeof AvatarImage; Fallback: typeof AvatarFallback; }; const Avatar: AvatarComponent = () => { console.warn('Direct usage of Avatar is not supported. Please use Avatar.Root, Avatar.Image, Avatar.Fallback instead.'); return null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/components/ui/Avatar/Avatar.tsx(1 hunks)src/components/ui/Avatar/contexts/AvatarContext.tsx(1 hunks)src/components/ui/Avatar/fragments/AvatarFallback.tsx(1 hunks)src/components/ui/Avatar/fragments/AvatarImage.tsx(1 hunks)src/components/ui/Avatar/fragments/AvatarRoot.tsx(1 hunks)src/components/ui/Avatar/stories/Avatar.stories.tsx(2 hunks)src/components/ui/Avatar/tests/Avatar.test.tsx(1 hunks)src/components/ui/AvatarGroup/AvatarGroup.tsx(1 hunks)src/components/ui/AvatarGroup/tests/AvatarGroup.test.js(1 hunks)src/components/ui/Badge/tests/Badge.test.tsx(1 hunks)src/components/ui/BlockQuote/tests/BlockQuote.test.tsx(1 hunks)src/components/ui/DataList/tests/DataList.test.tsx(2 hunks)src/components/ui/Quote/tests/Quote.test.tsx(1 hunks)src/components/ui/Toggle/tests/Toggle.test.js(1 hunks)src/core/customClassSwitcher/customClassSwitcher.test.ts(1 hunks)src/core/customClassSwitcher/index.ts(1 hunks)src/core/primitives/Avatar/contexts/AvatarPrimitiveContext.tsx(1 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx(1 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx(2 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveRoot.tsx(2 hunks)src/core/primitives/Avatar/stories/AvatarPrimitive.stories.tsx(0 hunks)src/core/primitives/Primitive/index.tsx(1 hunks)styles/themes/components/avatar.scss(1 hunks)
💤 Files with no reviewable changes (1)
- src/core/primitives/Avatar/stories/AvatarPrimitive.stories.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/components/ui/Avatar/fragments/AvatarFallback.tsx (2)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (1)
AvatarPrimitiveFallbackProps(4-8)src/components/ui/Avatar/contexts/AvatarContext.tsx (1)
AvatarContext(9-14)
src/core/customClassSwitcher/customClassSwitcher.test.ts (1)
src/core/customClassSwitcher/index.ts (1)
customClassSwitcher(7-20)
src/components/ui/Avatar/fragments/AvatarRoot.tsx (4)
src/core/primitives/Avatar/fragments/AvatarPrimitiveRoot.tsx (1)
AvatarPrimitiveRootProps(5-8)src/core/customClassSwitcher/index.ts (1)
customClassSwitcher(7-20)src/core/hooks/createDataAttribute/index.ts (3)
useCreateDataAttribute(10-25)useCreateDataAccentColorAttribute(48-55)useComposeAttributes(33-40)src/components/ui/Avatar/contexts/AvatarContext.tsx (1)
AvatarContext(9-14)
🪛 GitHub Check: lint
src/components/ui/Avatar/fragments/AvatarFallback.tsx
[warning] 5-5:
'AvatarPrimitiveFallbackProps' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (32)
src/components/ui/Toggle/tests/Toggle.test.js (1)
13-13: LGTM! Class naming convention updated correctlyThe update to expect the
customRootClassto be appended with-toggleis consistent with the changes to thecustomClassSwitcherutility.src/core/customClassSwitcher/customClassSwitcher.test.ts (1)
8-8: Test correctly updated to match new class naming conventionThe test expectation now correctly validates that the
customClassSwitcherfunction appends a kebab-cased component name to the custom root class, which aligns with the implementation changes to enforce consistent class naming across components.src/components/ui/Quote/tests/Quote.test.tsx (1)
25-25: LGTM! Updated test uses clearer naming exampleThe change from "custom-quote-class" to "acme-corp" with expectation of "acme-corp-quote" provides a clearer example of how the customClassSwitcher appends the component name to the custom root class.
Also applies to: 33-33
styles/themes/components/avatar.scss (3)
7-7: Border color changed to a darker grayThe border color has been changed from gray-300 to gray-600, making it more prominent. This is a visual change that will affect all avatar instances.
10-10: Class name updated to reflect new component structureThe class name has been changed from
.rad-ui-fallbackto.rad-ui-avatar-fallback, which aligns with the new modular component architecture. This is a good change for clarity and consistency.
13-13: Font size added to fallbackAdding a specific font size (16px) to the fallback ensures consistent text rendering across different environments and browser defaults.
src/core/primitives/Avatar/contexts/AvatarPrimitiveContext.tsx (1)
3-8: Context interface simplified appropriatelyThe
AvatarPrimitiveContextTypehas been simplified by removing styling-related properties (rootClass,fallBackRootClass, andsrc), which aligns with the separation of concerns pattern. The primitive context now focuses only on image loading state handling, which is a more appropriate scope.src/components/ui/Badge/tests/Badge.test.tsx (1)
15-15: Updated class naming convention for custom root classesThe test has been correctly updated to reflect the new class naming convention where component-specific suffixes are appended to custom root classes. This change is part of a larger standardization effort across the component library.
src/components/ui/BlockQuote/tests/BlockQuote.test.tsx (1)
15-15: Consistent update to class naming conventionThe test for BlockQuote follows the same pattern as the Badge test, correctly updating the expected class name to include the component-specific suffix (
-block-quote). This ensures consistency in how custom root classes are handled across components.src/core/primitives/Primitive/index.tsx (1)
4-4: Support forimgtag added to primitivesThe addition of 'img' to SUPPORTED_HTML_ELEMENTS enhances the Primitive component factory by enabling it to create a primitive component for the
<img>element. This change supports the Avatar component refactoring, allowing the use ofPrimitive.imginstead of direct HTML image tags.src/components/ui/AvatarGroup/AvatarGroup.tsx (2)
20-20: Properly removed color prop from AvatarGroupRemoving the color prop from the destructured properties aligns with the broader Avatar component refactoring that now handles styling through context and CSS classes rather than direct props.
25-25: Removed color prop from AvatarPrimitiveFallbackThe removal of the color prop from AvatarPrimitiveFallback is consistent with the new styling approach using context and CSS classes instead of explicit color props.
src/components/ui/DataList/tests/DataList.test.tsx (1)
44-44: Updated custom class naming convention in testsThe test has been properly updated to reflect the new naming convention for custom root classes, where component-specific suffixes (e.g.,
-data-list) are appended to the providedcustomRootClassvalue. This is consistent with the changes in thecustomClassSwitcherutility.Also applies to: 53-54, 56-57, 59-60, 62-63
src/components/ui/Avatar/contexts/AvatarContext.tsx (1)
9-14: LGTM! Clean context implementation with default values.The context is properly created with sensible default values, which helps prevent undefined errors when components consume the context before a provider is available.
src/components/ui/Avatar/fragments/AvatarFallback.tsx (1)
9-12: LGTM! Well-structured component with good composition pattern.The component correctly uses the context to obtain the
rootClassand composes it with a specific suffix for proper styling. Props and children are properly forwarded to the primitive component.src/components/ui/Avatar/fragments/AvatarImage.tsx (1)
8-11: LGTM! Clear type definition with sensible defaults.The component effectively extends the primitive props and provides reasonable defaults for
srcandalt.src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx (1)
11-12: LGTM! Consistent default values.Using empty strings as defaults for
srcandaltis consistent with how they're handled in the higher-level components.src/core/primitives/Avatar/fragments/AvatarPrimitiveRoot.tsx (2)
10-10: Good simplification of component parameters!The removal of
customRootClassandclassNameparameters from this primitive component appropriately shifts styling responsibilities to higher-level components, aligning with separation of concerns principles.
32-33: Clean implementation of primitive component!Removing the
classNameprop from thePrimitive.spanelement completes the transition to a more focused primitive component that handles only core functionality (image loading and error states) without styling concerns.src/components/ui/Avatar/tests/Avatar.test.tsx (2)
5-16: Great use of reusable test components!Creating
ComponentandFallbackComponentreduces duplication and makes tests more maintainable. These components properly demonstrate the new compositional API with Avatar.Root, Avatar.Image, and Avatar.Fallback.
49-53: Good update to color attribute testing!The test now correctly checks for the
data-rad-ui-accent-colorattribute on the parent element, which aligns with the new implementation where color is applied at the root level.src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (1)
1-11: Good simplification of the fallback component!Adding the
classNameprop while removing context dependencies simplifies this component to focus on rendering only. The component now accepts styling directly via props rather than computing it internally, which aligns with the architectural improvements for better separation of concerns.src/components/ui/Avatar/fragments/AvatarRoot.tsx (3)
10-16: Well-defined prop types for the new component!The AvatarRoot component properly extends AvatarPrimitiveRootProps while adding styling-related props (customRootClass, className, size, variant, color), creating a clear API for the root component in the new compositional pattern.
18-24: Excellent implementation of styling and attribute management!The component effectively:
- Uses customClassSwitcher to generate the root class name
- Creates data attributes for variant, size, and accent color
- Composes these attributes using utility hooks
This approach centralizes styling logic and keeps the component code clean and maintainable.
25-27: Great use of context for sharing styling information!Providing size, variant, color, and rootClass via context allows child components to access these values without prop drilling. The implementation correctly combines all styling and attributes when rendering the primitive component.
src/components/ui/Avatar/stories/Avatar.stories.tsx (4)
12-15: Good refactoring to compound component pattern.The story render function has been successfully updated to use the new compound component pattern with
Avatar.Root,Avatar.Image, andAvatar.Fallbackinstead of a monolithic Avatar component. This approach provides better composability and clarity.
23-26: Improved demonstration of Avatar properties.The story now shows more configuration options with the explicit props for
fallback,color,size, andvariant, which helps developers understand the available customization options.
30-33: Good addition of custom class demonstration.Adding a story specifically for custom root class usage helps document this feature for other developers.
64-69: Useful size demonstration story.The new story for size variations helps document and visualize this customization option.
src/components/ui/Avatar/Avatar.tsx (3)
4-6: Well-structured imports for fragment components.The imports are cleanly organized for the new fragment components that implement the compound pattern.
8-8: Good practice using a constant for component name.Using a constant for the component name ensures consistency across the component and helps with debugging.
10-13: Appropriate migration warning.The warning for direct usage helps guide developers to use the new compound component API correctly, providing a better developer experience during migration.
| export const customClassSwitcher = (customRootClass: string = '', componentName: string = ''): string => { | ||
| if (customRootClass) { | ||
| return customRootClass; | ||
| } | ||
|
|
||
| if (!componentName) { | ||
| return ''; | ||
| } | ||
|
|
||
| // add dashes between capitalized words | ||
| const componentClassName = componentName.replace(/([a-z0-9])([A-Z])/g, '$1-$2').toLowerCase(); | ||
|
|
||
| if (customRootClass) { | ||
| return `${customRootClass}-${componentClassName}`; | ||
| } | ||
|
|
||
| return `${RAD_UI_CLASS_PREFIX}-${componentClassName}`; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify potential impact on existing components
The change to append component names to custom root classes represents a breaking change in how customRootClass is handled. Ensure all components using customRootClass have been updated and tested with this new behavior.
🏁 Script executed:
#!/bin/bash
# Search for all components using customRootClass prop
rg -l "customRootClass(?:[ ]*)?(:[ ]*)?string" --type=typescript
# Check for any test files that might still be using the old convention
rg -l "expect\(.*\)\.toHaveClass\('custom-[^-]+" --type=typescriptLength of output: 208
🏁 Script executed:
#!/bin/bash
# Search for all usages of customRootClass prop across TS and TSX files
rg -l "customRootClass" -g "*.{ts,tsx}"
# Check for any test files using toHaveClass with the old 'custom-' prefix (no component suffix)
rg -l "toHaveClass\\('custom-" -g "*.{ts,tsx}"Length of output: 3348
Update tests and documentation for new customRootClass behavior
The change to always append the component’s kebab-case name to customRootClass alters the generated CSS class (e.g., from custom- to custom-quote). We’ve identified remaining references to the old convention in tests and docs that must be updated:
• Tests
- src/components/ui/Quote/tests/Quote.test.tsx
- src/components/ui/DataList/tests/DataList.test.tsx
UpdatetoHaveClass('custom-…')assertions to include the component suffix (e.g.,custom-quote,custom-data-list).
• Documentation examples
- docs/app/docs/components/**/docs/api/*.tsx
Review allcustomRootClass="custom"examples and append the component name (e.g.,custom-blockquote,custom-progress, etc.) to reflect the new output.
| // @ts-ignore | ||
| <Primitive.img src={src} alt={alt} onError={handleErrorImage} onLoad={handleLoadImage} {...props} /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove @ts-ignore and fix type definitions.
Using @ts-ignore suppresses all TypeScript errors, which can hide other issues. It would be better to properly type the Primitive.img component to ensure type safety.
Let's check if there are definitions for other Primitive elements that could serve as a template:
🏁 Script executed:
#!/bin/bash
# Check Primitive.tsx implementation
cat $(fd -t f "Primitive.tsx" --exec echo {})Length of output: 694
🏁 Script executed:
#!/bin/bash
# Locate and print AvatarPrimitiveImage.tsx
AVATAR_FILE=$(fd -t f AvatarPrimitiveImage.tsx)
echo "=== AvatarPrimitiveImage.tsx ==="
sed -n '1,200p' "$AVATAR_FILE"
# Show import lines for Primitive in the Avatar file
echo "=== Import lines for Primitive ==="
grep -n "import .*Primitive" "$AVATAR_FILE" || echo "No direct Primitive import found."
# Search for any module exporting 'Primitive' in src/core/primitives
echo "=== Files exporting 'Primitive' ==="
rg -l "export .*Primitive" -n src/core/primitives
# Search for any declaration of 'const Primitive'
echo "=== Declarations of 'const Primitive' ==="
rg -n "const Primitive" -n src/core/primitivesLength of output: 3118
#!/bin/bash
# Inspect PrimitiveProps and SUPPORTED_HTML_ELEMENTS in Primitive/index.tsx
PRIMITIVE_FILE="src/core/primitives/Primitive/index.tsx"
echo "=== Showing top of Primitive/index.tsx ==="
sed -n '1,200p' "$PRIMITIVE_FILE"
echo
echo "=== Lines defining PrimitiveProps ==="
grep -n "export type PrimitiveProps" -n "$PRIMITIVE_FILE" || echo "No direct export, searching 'type PrimitiveProps'"
grep -n "type PrimitiveProps" -n "$PRIMITIVE_FILE"
echo
echo "=== SUPPORTED_HTML_ELEMENTS ==="
grep -n "SUPPORTED_HTML_ELEMENTS" -n "$PRIMITIVE_FILE"🏁 Script executed:
#!/bin/bash
# Locate and inspect Primitive/index.tsx for type definitions
PRIMITIVE_PATH=$(fd -t f "index.tsx" src/core/primitives/Primitive)
echo "=== Primitive/index.tsx path: $PRIMITIVE_PATH ==="
sed -n '1,200p' "$PRIMITIVE_PATH"
echo
echo "=== Searching for PrimitiveProps ==="
grep -n "type PrimitiveProps" "$PRIMITIVE_PATH" || echo "No 'type PrimitiveProps' found."
echo
echo "=== Searching for SUPPORTED_HTML_ELEMENTS ==="
grep -n "SUPPORTED_HTML_ELEMENTS" "$PRIMITIVE_PATH" || echo "No 'SUPPORTED_HTML_ELEMENTS' found."Length of output: 4213
Refactor Primitive for element-specific props and remove @ts-ignore
The @ts-ignore is currently needed because PrimitiveProps is defined as React.HTMLAttributes<HTMLElement>, which doesn’t include image-specific attributes (src, alt, onError, onLoad). To restore full type safety:
• src/core/primitives/Primitive/index.tsx
- Change
interface PrimitivePropsto a generic mapping, e.g.:type PrimitiveProps<T extends SupportedElement> = React.ComponentPropsWithoutRef<T> & { asChild?: boolean; };
- Update
createPrimitiveComponentsignature to<T extends SupportedElement>(elementType: T) => React.ForwardRefExoticComponent<PrimitiveProps<T> & React.RefAttributes<React.ElementRef<T>>>; - In the
reducecall, use the genericPrimitiveProps<T>and the correctReact.ElementRef<T>for each element.
• src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx
- After the above refactor, remove the
// @ts-ignoreon the<Primitive.img … />line so that image props are properly type-checked.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/components/ui/AvatarGroup/tests/AvatarGroup.test.js (1)
66-70: Remove or update commented-out testThis commented-out test was likely testing color attributes for fallbacks, but hasn't been updated to work with the new component structure.
Either remove this commented-out test or update it to work with the new component pattern:
- // test('renders color for fallback when src is not provided', async() => { - // render(<AvatarGroup avatars={avatarsWithFallback} color='blue'/>); - // expect(screen.getByText('A')).toHaveAttribute('data-rad-ui-accent-color', 'blue'); - // expect(screen.getByText('B')).toHaveAttribute('data-rad-ui-accent-color', 'blue'); - // }); + test('renders color for fallback when src is not provided', async() => { + render(<AvatarGroup.Root> + <AvatarGroup.Item color='blue'> + <AvatarGroup.Avatar src='' alt='Avatar 1' /> + <AvatarGroup.Fallback>A</AvatarGroup.Fallback> + </AvatarGroup.Item> + <AvatarGroup.Item color='blue'> + <AvatarGroup.Avatar src='' alt='Avatar 2' /> + <AvatarGroup.Fallback>B</AvatarGroup.Fallback> + </AvatarGroup.Item> + </AvatarGroup.Root>); + expect(screen.getByText('A').parentElement).toHaveAttribute('data-rad-ui-accent-color', 'blue'); + expect(screen.getByText('B').parentElement).toHaveAttribute('data-rad-ui-accent-color', 'blue'); + });src/components/ui/AvatarGroup/fragments/AvatarGroupItem.tsx (1)
8-8: Consider extracting props interface for better maintainabilityThe component's props are defined inline within the function parameters, which works but could be improved for maintainability as the component evolves.
Extract the props to a separate interface:
+interface AvatarGroupItemProps { + color?: string; + children: React.ReactNode; +} -const AvatarGroupItem = ({ color = '', children }: { color?: string, children: React.ReactNode }) => { +const AvatarGroupItem = ({ color = '', children }: AvatarGroupItemProps) => {src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx (1)
4-5: Unused import detectedThe
useCreateDataAccentColorAttributehook is imported but never used in this component.Remove the unused import:
-import { useCreateDataAttribute, useComposeAttributes, useCreateDataAccentColorAttribute } from '~/core/hooks/createDataAttribute'; +import { useCreateDataAttribute, useComposeAttributes } from '~/core/hooks/createDataAttribute';🧰 Tools
🪛 GitHub Check: lint
[warning] 5-5:
'useCreateDataAccentColorAttribute' is defined but never usedstyles/themes/components/avatar-group.scss (2)
8-9: Remove duplicate position property.The
position: relativeproperty is mistakenly duplicated on lines 8 and 9.position: relative; - position: relative;
10-12: Consider making dimensions configurable via CSS variables.The hardcoded dimensions (36px) limit the flexibility of the component. Consider using CSS variables for width and height to support different sizes.
- width:36px; - height:36px; + width: var(--rad-ui-avatar-group-item-size, 36px); + height: var(--rad-ui-avatar-group-item-size, 36px);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/components/ui/Avatar/stories/Avatar.stories.tsx(2 hunks)src/components/ui/AvatarGroup/AvatarGroup.tsx(1 hunks)src/components/ui/AvatarGroup/contexts/AvatarGroupContext.tsx(1 hunks)src/components/ui/AvatarGroup/fragments/AvatarGroupAvatar.tsx(1 hunks)src/components/ui/AvatarGroup/fragments/AvatarGroupFallback.tsx(1 hunks)src/components/ui/AvatarGroup/fragments/AvatarGroupItem.tsx(1 hunks)src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx(1 hunks)src/components/ui/AvatarGroup/stories/AvatarGroup.stories.tsx(2 hunks)src/components/ui/AvatarGroup/tests/AvatarGroup.test.js(1 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx(1 hunks)src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx(1 hunks)styles/themes/components/avatar-group.scss(1 hunks)styles/themes/components/avatar.scss(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/components/ui/AvatarGroup/fragments/AvatarGroupAvatar.tsx
- src/components/ui/AvatarGroup/fragments/AvatarGroupFallback.tsx
- src/components/ui/AvatarGroup/contexts/AvatarGroupContext.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- styles/themes/components/avatar.scss
- src/core/primitives/Avatar/fragments/AvatarPrimitiveImage.tsx
- src/components/ui/Avatar/stories/Avatar.stories.tsx
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (1)
src/core/primitives/Avatar/contexts/AvatarPrimitiveContext.tsx (1)
AvatarPrimitiveContext(10-10)
src/components/ui/AvatarGroup/AvatarGroup.tsx (1)
docs/app/docs/components/avatar-group/docs/avatarGroup_anatomy.tsx (1)
AvatarGroup(3-12)
src/components/ui/AvatarGroup/fragments/AvatarGroupItem.tsx (2)
src/components/ui/AvatarGroup/contexts/AvatarGroupContext.tsx (1)
AvatarGroupContext(3-7)src/core/hooks/createDataAttribute/index.ts (2)
useCreateDataAccentColorAttribute(48-55)useComposeAttributes(33-40)
src/components/ui/AvatarGroup/stories/AvatarGroup.stories.tsx (1)
docs/app/docs/components/avatar-group/docs/avatarGroup_anatomy.tsx (1)
AvatarGroup(3-12)
🪛 GitHub Check: lint
src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx
[warning] 5-5:
'useCreateDataAccentColorAttribute' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx (2)
3-3: Improved import specificity.The import is now more specific, pointing directly to the context file, which improves code clarity and maintainability.
8-8: Simplified className prop type.The className prop has been simplified to an optional string, removing unnecessary complexity. This aligns with the refactoring goal of moving styling responsibilities to higher-level components.
src/components/ui/AvatarGroup/stories/AvatarGroup.stories.tsx (1)
41-41: Story name changed to be more descriptiveRenaming from
withFallbacktowithBrokenSrcFallbackprovides better clarity about what the story is demonstrating.src/components/ui/AvatarGroup/tests/AvatarGroup.test.js (1)
5-20: Test updated to use the new compound component patternThe test now uses a helper
Compcomponent that implements the new compound component pattern, making the tests align with the component's updated API.src/components/ui/AvatarGroup/fragments/AvatarGroupItem.tsx (1)
1-19: Well-structured component with clear responsibilityThis component correctly implements the AvatarGroupItem fragment of the compound component pattern. It handles accent color attributes and leverages context to maintain consistent styling across the component hierarchy.
src/components/ui/AvatarGroup/fragments/AvatarGroupRoot.tsx (3)
14-14: Good use of constant for component nameUsing a constant for the component name improves maintainability and reduces the risk of typos.
22-26: Properly implemented context provider with essential valuesThe component now provides size, variant, and rootClass through context, allowing child components to access these values without prop drilling. This is a good pattern for compound components.
18-19: Well-implemented data attributes for variant and sizeThe component correctly uses hooks to create and compose data attributes for variant and size, ensuring consistent styling and behavior.
styles/themes/components/avatar-group.scss (2)
4-4: Good addition for vertical alignment.Adding
align-items: centerensures all items are properly aligned vertically.
17-32: Good modular structure for image and fallback styling.The new classes for images and fallbacks create a clear separation of concerns and follow proper component composition patterns.
src/components/ui/AvatarGroup/AvatarGroup.tsx (2)
6-9: Good pattern for deprecating direct usage.Providing a warning for direct component usage guides developers toward the new composition pattern.
Consider enhancing the warning message with a link to migration documentation:
- console.warn('Direct usage of AvatarGroup is not supported. Please use AvatarGroup.Root, AvatarGroup.Item, AvatarGroup.Avatar, AvatarGroup.Fallback instead.'); + console.warn('Direct usage of AvatarGroup is not supported. Please use AvatarGroup.Root, AvatarGroup.Item, AvatarGroup.Avatar, AvatarGroup.Fallback instead. See documentation for migration guide: [URL]');
1-4: Good component fragmentation approach.Breaking down the component into smaller, focused fragments (Root, Item, Avatar, Fallback) follows best practices for composable React components.
| const AvatarPrimitiveFallback = ({ children, asChild = false, className = '', ...props }: AvatarPrimitiveFallbackProps) => { | ||
| const { hasError, isImageLoaded } = useContext(AvatarPrimitiveContext); | ||
| if (isImageLoaded) return null; | ||
| return <Primitive.span asChild={asChild} className={className} {...props}>{children}</Primitive.span>; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Streamlined component implementation with improved conditional rendering.
The implementation has been significantly improved with:
- Default values for optional props
- Cleaner context usage with destructuring
- Simplified conditional rendering based on
isImageLoadedstate - More direct props passing to the underlying Primitive.span
There's an unused hasError variable being destructured from the context. Is this intentional or should it be used in the conditional logic?
🏁 Script executed:
#!/bin/bash
# Check if hasError is used elsewhere in the codebase or if it's potentially needed for future use
rg -A 5 -B 5 "hasError" --glob "*.{tsx,ts,jsx,js}" | grep -v "const { hasError" | grep -v "hasError,"Length of output: 5442
Remove unused hasError from AvatarPrimitiveFallback
The hasError value is destructured from context but never used in this component. To keep the implementation clean, remove it:
Locations:
- src/core/primitives/Avatar/fragments/AvatarPrimitiveFallback.tsx
Suggested diff:
- const { hasError, isImageLoaded } = useContext(AvatarPrimitiveContext);
+ const { isImageLoaded } = useContext(AvatarPrimitiveContext);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const AvatarPrimitiveFallback = ({ children, asChild = false, className = '', ...props }: AvatarPrimitiveFallbackProps) => { | |
| const { hasError, isImageLoaded } = useContext(AvatarPrimitiveContext); | |
| if (isImageLoaded) return null; | |
| return <Primitive.span asChild={asChild} className={className} {...props}>{children}</Primitive.span>; | |
| const AvatarPrimitiveFallback = ({ children, asChild = false, className = '', ...props }: AvatarPrimitiveFallbackProps) => { | |
| const { isImageLoaded } = useContext(AvatarPrimitiveContext); | |
| if (isImageLoaded) return null; | |
| return <Primitive.span asChild={asChild} className={className} {...props}>{children}</Primitive.span>; | |
| } |
| <AvatarGroup.Root size='large' variant='circle' > | ||
| <AvatarGroup.Item color='blue'> | ||
| <AvatarGroup.Avatar src={args.avatars[0].src} alt={args.avatars[0].fallback} /> | ||
| <AvatarGroup.Fallback>{args.avatars[0].fallback}</AvatarGroup.Fallback> | ||
| </AvatarGroup.Item> | ||
| <AvatarGroup.Item color='red'> | ||
| <AvatarGroup.Avatar src={args.avatars[1].src} alt={args.avatars[1].fallback} /> | ||
| <AvatarGroup.Fallback>{args.avatars[1].fallback}</AvatarGroup.Fallback> | ||
| </AvatarGroup.Item> | ||
| <AvatarGroup.Item color='green'> | ||
| <AvatarGroup.Avatar src={args.avatars[2].src} alt={args.avatars[2].fallback} /> | ||
| <AvatarGroup.Fallback>{args.avatars[2].fallback}</AvatarGroup.Fallback> | ||
| </AvatarGroup.Item> | ||
| </AvatarGroup.Root> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
API changed from prop-based to composition-based pattern
The AvatarGroup component has been updated to use a compound component pattern with explicit subcomponents. This provides more flexibility and better separation of concerns, but represents a breaking change for existing consumers.
Make sure to update all existing usage sites throughout the codebase to use this new pattern. Consider providing a temporary backwards compatibility layer that supports both APIs during a transition period.
| <AvatarGroup.Root size='large' variant='circle' > | ||
| <AvatarGroup.Item color='blue'> | ||
| <AvatarGroup.Avatar src={args.avatars[0].src} alt={args.avatars[0].fallback} /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Inconsistency between story implementation and documentation
The story implementation uses AvatarGroup.Item while the documentation in docs/app/docs/components/avatar-group/docs/avatarGroup_anatomy.tsx uses AvatarGroup.AvatarRoot. This inconsistency could lead to confusion for developers.
Verify that the component naming is consistent across all files:
🏁 Script executed:
#!/bin/bash
# Check for component naming inconsistencies across files
echo "Checking AvatarGroup component naming consistency..."
rg -A 1 "AvatarGroup\.(Item|AvatarRoot)" --type tsxLength of output: 196
🏁 Script executed:
#!/bin/bash
# Re-run search for AvatarGroup component naming usages across .tsx and .ts files
echo "Searching for AvatarGroup.AvatarRoot and AvatarGroup.Item in .ts/.tsx files..."
rg -Hn "AvatarGroup\.(AvatarRoot|Item)" --glob "*.{ts,tsx}"Length of output: 1356
Align AvatarGroup root component naming
There’s an inconsistency between your story and the documentation:
- In
src/components/ui/AvatarGroup/stories/AvatarGroup.stories.tsx(around line 13):
<AvatarGroup.Root size='large' …> - In
docs/app/docs/components/avatar-group/docs/avatarGroup_anatomy.tsx(lines 6–9):
<AvatarGroup.AvatarRoot>…</AvatarGroup.AvatarRoot>
Please update one of them so both examples use the same root component name (for instance, change the docs to <AvatarGroup.Root>).
| fireEvent.error(img); | ||
| // Assert that the fallback text is rendered | ||
| expect(screen.getByText(avatar.fallback)).toBeInTheDocument(); | ||
| expect(screen.getByText('A')).toBeInTheDocument(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broken image test should verify all fallbacks
The test for broken images is only checking for the first fallback text "A" even though multiple images are failing.
Update the test to verify all fallbacks are shown when their respective images fail:
test('AvatarGroup renders for broken image src', () => {
render(<Comp />);
const avatarImages = screen.getAllByRole('img');
- avatarImages.forEach(img => {
+ avatarImages.forEach((img, index) => {
fireEvent.error(img);
// Assert that the fallback text is rendered
- expect(screen.getByText('A')).toBeInTheDocument();
+ expect(screen.getByText(['A', 'B', 'C'][index])).toBeInTheDocument();
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fireEvent.error(img); | |
| // Assert that the fallback text is rendered | |
| expect(screen.getByText(avatar.fallback)).toBeInTheDocument(); | |
| expect(screen.getByText('A')).toBeInTheDocument(); | |
| test('AvatarGroup renders for broken image src', () => { | |
| render(<Comp />); | |
| const avatarImages = screen.getAllByRole('img'); | |
| avatarImages.forEach((img, index) => { | |
| fireEvent.error(img); | |
| // Assert that the fallback text is rendered | |
| expect(screen.getByText(['A', 'B', 'C'][index])).toBeInTheDocument(); | |
| }); | |
| }); |
| AvatarGroup.Item = AvatarGroupItem; | ||
| AvatarGroup.Avatar = AvatarGroupAvatar; | ||
| AvatarGroup.Fallback = AvatarGroupFallback; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API breaking change detected.
The component API has changed significantly. The previous API used AvatarGroup.AvatarRoot, AvatarGroup.AvatarImage, and AvatarGroup.AvatarFallback (as seen in the relevant code snippet), while the new API exports AvatarGroup.Item, AvatarGroup.Avatar, and AvatarGroup.Fallback.
This change will require updates to existing code that uses the AvatarGroup component.
Given this breaking change:
- Consider documenting a migration path for existing users
- Consider adding a temporary backwards compatibility layer
- Ensure that all usages in the codebase have been updated
🏁 Script executed:
#!/bin/bash
# Find usages of the old AvatarGroup API in the codebase
echo "Searching for usages of old AvatarGroup.AvatarRoot API..."
rg "AvatarGroup\.AvatarRoot" -t tsx -t jsx
echo "Searching for usages of old AvatarGroup.AvatarImage API..."
rg "AvatarGroup\.AvatarImage" -t tsx -t jsx
echo "Searching for usages of old AvatarGroup.AvatarFallback API..."
rg "AvatarGroup\.AvatarFallback" -t tsx -t jsxLength of output: 608
🏁 Script executed:
#!/bin/bash
# Re-run searches for old AvatarGroup API without relying on unknown file types
echo "Searching for usages of old AvatarGroup.AvatarRoot API..."
rg "AvatarGroup\.AvatarRoot" -g "*.tsx" -g "*.jsx"
echo "Searching for usages of old AvatarGroup.AvatarImage API..."
rg "AvatarGroup\.AvatarImage" -g "*.tsx" -g "*.jsx"
echo "Searching for usages of old AvatarGroup.AvatarFallback API..."
rg "AvatarGroup\.AvatarFallback" -g "*.tsx" -g "*.jsx"Length of output: 982
Update documentation and migration support for AvatarGroup API changes
The documentation still references the old API in docs/app/docs/components/avatar-group/docs/avatarGroup_anatomy.tsx. Please:
- Replace old component names:
- <AvatarGroup.AvatarRoot> + <AvatarGroup.Item> … - <AvatarGroup.AvatarImage /> + <AvatarGroup.Avatar /> … - <AvatarGroup.AvatarFallback /> + <AvatarGroup.Fallback />
- Add or update a migration guide outlining the mapping from the old API (
AvatarRoot,AvatarImage,AvatarFallback) to the new one (Item,Avatar,Fallback). - (Optional) Introduce a temporary backwards-compatibility layer that aliases the old exports to the new ones with deprecation warnings, to ease the transition for existing users.
Committable suggestion skipped: line range outside the PR's diff.
|
#yolo |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Style
Tests
Chores