-
-
Notifications
You must be signed in to change notification settings - Fork 53
AvatarPrimitive Component + Base AvatarGroup component #425
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
Changes from all commits
69b799a
2556d98
3c93ac8
9dda59b
bc71437
3834ee1
e677196
0ab2b4e
052e3a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import React from 'react'; | ||
|
|
||
| import AvatarGroupRoot from './shards/AvatarGroupRoot'; | ||
|
|
||
| const COMPONENT_NAME = 'AvatarGroup'; | ||
|
|
||
| // contexts | ||
|
|
||
| // export type AvatarProps = { | ||
| // children?: React.ReactNode, | ||
| // customRootClass?: string, | ||
| // fallback?: string, | ||
| // className?: string, | ||
| // src?: string, | ||
| // alt?: string, | ||
| // props?: Record<string, any>[] | ||
| // } | ||
|
|
||
|
Comment on lines
+9
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Define and implement proper TypeScript interfaces. The component lacks proper type definitions which are crucial for type safety and developer experience.
export interface AvatarProps {
children?: React.ReactNode;
customRootClass?: string;
fallback?: string;
className?: string;
src?: string;
alt?: string;
props?: Record<string, unknown>; // More specific than any
}
export interface AvatarGroupProps {
avatars: AvatarProps[];
size?: 'sm' | 'md' | 'lg';
customRootClass?: string;
className?: string;
} |
||
| const AvatarGroup = ({ avatars = [], size, customRootClass = '', className }) => { | ||
| return <AvatarGroupRoot> | ||
|
|
||
| </AvatarGroupRoot>; | ||
| }; | ||
|
Comment on lines
+19
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implement avatar rendering logic and type the component. The component is currently a shell without implementation. It needs proper typing and rendering logic. Here's a suggested implementation: -const AvatarGroup = ({ avatars = [], size, customRootClass = '', className }) => {
+const AvatarGroup: FC<AvatarGroupProps> = ({
+ avatars = [],
+ size = 'md',
+ customRootClass = '',
+ className = ''
+}) => {
+ const avatarElements = avatars.map((avatar, index) => (
+ <Avatar
+ key={index}
+ size={size}
+ {...avatar}
+ />
+ ));
+
return (
- <AvatarGroupRoot>
-
+ <AvatarGroupRoot
+ className={`avatar-group ${customRootClass} ${className}`.trim()}
+ >
+ {avatarElements}
</AvatarGroupRoot>
);
};
|
||
|
|
||
| AvatarGroup.displayName = COMPONENT_NAME; | ||
|
|
||
| export default AvatarGroup; | ||
|
Comment on lines
+25
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Component requires test coverage and documentation The component lacks:
Please add:
🔗 Analysis chainVerify component documentation and testing. The component needs proper documentation and testing coverage. Let's check for related test and story files: Would you like me to help generate:
Let me try another search pattern to verify the component's documentation and testing coverage. Let me try one more search without the file type restriction: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for test and story files
echo "Checking for test and story files..."
fd -e test.tsx -e stories.tsx "AvatarGroup" src/
Length of output: 131 Script: #!/bin/bash
# Search for AvatarGroup related files with different extensions
echo "Searching for AvatarGroup related files..."
fd "AvatarGroup" src/
# Search for potential test references
echo -e "\nSearching for AvatarGroup in test files..."
rg "AvatarGroup" -t tsx --glob "*test*"
# Search for potential story references
echo -e "\nSearching for AvatarGroup in story files..."
rg "AvatarGroup" -t tsx --glob "*stories*"
Length of output: 655 Script: #!/bin/bash
# Search for test and story files without type restriction
echo "Searching in test files..."
rg "AvatarGroup" --glob "*test*"
echo -e "\nSearching in story files..."
rg "AvatarGroup" --glob "*stories*"
# Also check if there's any documentation in README or docs
echo -e "\nSearching in documentation..."
rg "AvatarGroup" --glob "*.md"
Length of output: 308 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { createContext } from 'react'; | ||
|
|
||
| const AvatarGroupContext = createContext({}); | ||
|
|
||
| export default AvatarGroupContext; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import AvatarGroupContext from './contexts/AvatarGroupContext'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| const AvatarGroupRoot = () => { | ||||||||||||||||||||||||||||||||||||||||||||
kotAPI marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||
| <div> | ||||||||||||||||||||||||||||||||||||||||||||
| <AvatarGroupContext.Provider value={{}}> | ||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initialize context with proper default values The context provider is initialized with an empty object, which could lead to runtime errors if consumers try to access undefined properties. Define and use proper context value: - <AvatarGroupContext.Provider value={{}}>
+ <AvatarGroupContext.Provider
+ value={{
+ // Add required context properties here
+ avatarCount: 0,
+ maxDisplayed: 5,
+ // ... other necessary values
+ }}
+ >
|
||||||||||||||||||||||||||||||||||||||||||||
| <AvatarGroupRoot > | ||||||||||||||||||||||||||||||||||||||||||||
| </AvatarGroupRoot> | ||||||||||||||||||||||||||||||||||||||||||||
| </AvatarGroupContext.Provider> | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential Infinite Recursion in Component Definition The Suggested change to prevent recursion: - <AvatarGroupRoot >
- </AvatarGroupRoot>
+ {/* Nested components should go here */}Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| export default AvatarGroupRoot; | ||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,28 @@ | ||
| import React, { DetailedHTMLProps, InputHTMLAttributes, PropsWithChildren} from 'react'; | ||
| import { customClassSwitcher} from "~/core"; | ||
| import React, { DetailedHTMLProps, InputHTMLAttributes, PropsWithChildren } from 'react'; | ||
| import { customClassSwitcher } from '~/core'; | ||
| import RadioPrimitive from '~/core/primitives/Radio'; | ||
| const COMPONENT_NAME = 'RadioGroup'; | ||
|
|
||
| export type RadioGroupProps = { | ||
|
|
||
| children?: React.ReactNode; | ||
| className: string; | ||
| customRootClass: string; | ||
| customRootClass: string; | ||
kotAPI marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } & DetailedHTMLProps<InputHTMLAttributes<HTMLInputElement>, HTMLInputElement> & PropsWithChildren | ||
|
|
||
| const RadioGroup =({children,type='radio', className='',customRootClass='', ...props}:RadioGroupProps) => { | ||
| const rootClass= customClassSwitcher(customRootClass,COMPONENT_NAME) | ||
| const RadioGroup = ({ children, type = 'radio', className = '', customRootClass = '', ...props }:RadioGroupProps) => { | ||
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||
|
|
||
| return ( | ||
| <div className={`${rootClass} ${className}`} role='radiogroup'> | ||
| <RadioPrimitive | ||
| type={type} | ||
| {...props}> | ||
| <div className={`${rootClass} ${className}`} role='radiogroup'> | ||
| <RadioPrimitive | ||
| type={type} | ||
| {...props}> | ||
|
|
||
| {children} | ||
| </RadioPrimitive> | ||
| </div> | ||
kotAPI marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ); | ||
| }; | ||
|
|
||
| {children} | ||
| </RadioPrimitive> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| export default RadioGroup; | ||
| export default RadioGroup; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import { createContext } from 'react'; | ||
|
|
||
| interface AvatarPrimitiveContextType { | ||
| rootClass: string; | ||
| fallBackRootClass: string; | ||
| isImageLoaded: boolean; | ||
| hasError: boolean; | ||
| handleLoadImage: () => void; | ||
| handleErrorImage: () => void; | ||
| src?: string; | ||
| } | ||
|
Comment on lines
+3
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance type definitions for better maintainability. A few suggestions to improve the interface:
type AvatarPrimitiveContextType = {
rootClass: string;
fallBackRootClass: string;
} & (
| { status: 'loading'; src: string }
| { status: 'loaded'; src: string }
| { status: 'error'; src: string }
| { status: 'idle'; src?: never }
);
|
||
|
|
||
| export const AvatarPrimitiveContext = createContext<AvatarPrimitiveContextType>({} as AvatarPrimitiveContextType); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve type safety of context initialization. The current implementation uses type assertion with an empty object, which could lead to runtime errors if context is accessed before provider is set up. Consider this safer implementation: -export const AvatarPrimitiveContext = createContext<AvatarPrimitiveContextType>({} as AvatarPrimitiveContextType);
+export const AvatarPrimitiveContext = createContext<AvatarPrimitiveContextType | null>(null);
+
+export function useAvatarPrimitiveContext(): AvatarPrimitiveContextType {
+ const context = useContext(AvatarPrimitiveContext);
+ if (context === null) {
+ throw new Error('useAvatarPrimitiveContext must be used within an AvatarPrimitiveProvider');
+ }
+ return context;
+}This approach:
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import React, { useContext } from 'react'; | ||
| import { AvatarPrimitiveContext } from '../contexts/AvatarPrimitiveContext'; | ||
|
|
||
| export interface AvatarPrimitiveFallbackProps { | ||
| children: React.ReactNode; | ||
| className?: string | ''; | ||
| } | ||
kotAPI marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const AvatarPrimitiveFallback = ({ children, className = '' }: AvatarPrimitiveFallbackProps) => { | ||
| const { hasError, fallBackRootClass } = useContext(AvatarPrimitiveContext); | ||
|
|
||
| if (!hasError) { | ||
| return null; | ||
| } | ||
|
|
||
| return <span className={`${fallBackRootClass} ${className}`}>{children}</span>; | ||
| }; | ||
kotAPI marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| export default AvatarPrimitiveFallback; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||||||||||||||||||||||||
| import React, { useContext } from 'react'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import { AvatarPrimitiveContext } from '../contexts/AvatarPrimitiveContext'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export interface AvatarRootImageProps extends React.ImgHTMLAttributes<HTMLImageElement> { | ||||||||||||||||||||||||||||
| src?: string; | ||||||||||||||||||||||||||||
| alt?: string; | ||||||||||||||||||||||||||||
| className?: string | ''; | ||||||||||||||||||||||||||||
| status?: 'loading' | 'loaded' | 'error'; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unused status prop from interface. The export interface AvatarRootImageProps extends React.ImgHTMLAttributes<HTMLImageElement> {
src?: string;
alt?: string;
className?: string | '';
- status?: 'loading' | 'loaded' | 'error';
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const AvatarPrimitiveImage = ({ | ||||||||||||||||||||||||||||
| src, | ||||||||||||||||||||||||||||
| alt = 'Avatar', | ||||||||||||||||||||||||||||
| className = '', | ||||||||||||||||||||||||||||
| ...props | ||||||||||||||||||||||||||||
| }: AvatarRootImageProps) => { | ||||||||||||||||||||||||||||
| const { handleErrorImage, handleLoadImage, hasError } = useContext(AvatarPrimitiveContext); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // If there's no src or there's an error, render nothing | ||||||||||||||||||||||||||||
| if (!src || hasError) { | ||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding explicit error state handling. While returning null is a valid approach, consider implementing an error boundary or providing feedback about why the image isn't rendered for better debugging and user experience. // If there's no src or there's an error, render nothing
if (!src || hasError) {
+ if (process.env.NODE_ENV !== 'production') {
+ console.warn(
+ `AvatarPrimitiveImage: Image not rendered due to ${!src ? 'missing src' : 'error state'}`
+ );
+ }
return null;
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||
| <img | ||||||||||||||||||||||||||||
| src={src} | ||||||||||||||||||||||||||||
| alt={alt} | ||||||||||||||||||||||||||||
| onError={handleErrorImage} | ||||||||||||||||||||||||||||
| onLoad={handleLoadImage} | ||||||||||||||||||||||||||||
| className={className} | ||||||||||||||||||||||||||||
| {...props} | ||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
|
Comment on lines
+25
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add loading optimization and ensure props safety. Consider adding loading optimization attributes and ensure important props aren't accidentally overridden. return (
<img
src={src}
alt={alt}
onError={handleErrorImage}
onLoad={handleLoadImage}
className={className}
+ loading="lazy"
+ decoding="async"
- {...props}
+ {...props}
+ // Ensure these props can't be overridden
+ onError={handleErrorImage}
+ onLoad={handleLoadImage}
/>
);
|
||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| export default AvatarPrimitiveImage; | ||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.