-
Notifications
You must be signed in to change notification settings - Fork 800
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
Adding support for framework image component in Radix Avatar #2999
base: main
Are you sure you want to change the base?
Conversation
Thanks for the suggestion! I'm not quite sure the implementation here is what we're looking for. A few things: First: we shouldn't need the <Avatar.Image asChild src="https://whatever.com/img.jpg" alt="cute kitten">
<NextImage width={500} height={300} />
</Avatar.Image> While you might get type errors since Next requires
Not everyone uses Next JS, and we shouldn't assume as much. I think I'd rather have a boolean prop that disables our internal optimizations rather than removing them completely. |
New to Next, but I found that supplying <AvatarImage src="/images/blank.png" asChild>
<Image alt={user.username} width={48} height={48} src={user.image} />
</AvatarImage> |
I've improved this code and raised PR in @RubensKj's fork. Changes: |
* 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
…ature/adding-avatar-component
@Udhayarajan thanks for the PR! I've merged and also fixed the merge problems with main branch. Thanks for contributing together 🚀 . Now let's wait for the review ❤️ |
onError: (event: React.SyntheticEvent<HTMLImageElement, Event>) => { | ||
console.log('error'); | ||
handleLoadingStatusChange('error'); | ||
if (childProps.onError) childProps.onError(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: @chaance do we also need to trigger onError
callback from props
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me make sense, considering that the child component might have some implementation of it 🤔.
But Chance can give us a better overview if it's necessary or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, to mimic the natural CSS flow, we should check and send the event to both the child's and parent's callback, so that if any using the parent callback the code won't break thus avoiding chaos.
* 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`
Description
Hello there, how you guys doing?
As discussed in issue #2230, we would love to enhance image optimization by integrating the Next.js Image component. I've implemented a solution that allows us to utilize the Next.js Image component while maintaining the existing functionality.
In this update, I've removed the image load logic from useLayoutEffect and delegated it directly to the respective components. This means that Primitive.img will handle image loading as usual, while the Next.js Image component will load images with its optimizations.
I would greatly appreciate your feedback on this solution. Let's collaborate to refine this feature and ensure it meets our needs.