Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

RubensKj
Copy link

@RubensKj RubensKj commented Jul 4, 2024

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.

@RubensKj RubensKj changed the title Adding feature for support framework image components Adding support for framework image component in Radix Avatar Jul 4, 2024
@chaance
Copy link
Member

chaance commented Aug 16, 2024

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 component prop since we already have asChild. This is our preferred API for polymorphic components, and you can do this today:

<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 src and alt, you should be able to suppress them since we merge props internally when we clone the child element.

while the Next.js Image component will load images with its optimizations

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.

@dragospaulpop
Copy link

dragospaulpop commented Sep 6, 2024

New to Next, but I found that supplying src to Avatar.Image also downloads the original file, which defeats the purpose of the optimizations provided by NextImage. I got it working by supplying AvatarImage with a small 1x1 transparent image and NextImage with the actual image url:

<AvatarImage src="/images/blank.png" asChild>
  <Image alt={user.username} width={48} height={48} src={user.image}  />
</AvatarImage>

@Udhayarajan
Copy link

Udhayarajan commented Sep 13, 2024

I've improved this code and raised PR in @RubensKj's fork.
Let's wait for a week, if he isn't active I will rebase the changes from this repository and raise PR here.

Changes:
RubensKj#1

* 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
@RubensKj
Copy link
Author

@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);

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?

Copy link
Author

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

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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants