-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Shadcn migration - image #13601
Shadcn migration - image #13601
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* TODO: Rename this component to `Image` once all components are using Tailwind | ||
*/ | ||
export const TwImage = (props: NextImageProps) => ( | ||
<DefaultNextImage {...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.
@pettinarip How do we want to handle any image sources that are external to the repo? Events, for example, will now be displaying external hosted images. To my knowledge, we have have a few options for rendering these:
- Allowlist the image domain and use Next Image (which we wouldn't necessarily know)
- Download the images in a separate task and import directly
- Or just use the
img
element without the Next wrapper (foregoing any optimization Next offers)
Any thoughts on this, and do you think we should add a touch of logic here to differentiate the image source, and avoid the Next Image component if needed? Or should we just use img
directly in those cases, and not bother importing this component? We currently get eslint @next/next/no-img-element
warnings when doing this.
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.
Good question, I don't have an easy answer. I guess it depends.
Before I continue, I don't think this needs to be part of the scope of this PR. This PR just exports the default next image component to keep the current implementation and help with the UI transition.
- If we know in advance where these images are coming from, I would lean towards adding these domains to the allowlist. But I would only do that in the case where we only have a few domains and we don't need to maintain too much (I would not expect that to happen).
- Using the
img
seems like the easiest solution, but we lack optimizations & security. - and downloading images will require some prep work + maintaining the images...
So I would choose one of the following options:
- I would choose the first option if the domains are predefined and don't change.
- I would store the images in a separate cloud service and consume them from there (adding that to the allowlist).
Description
Exports the default image component to be used in the UI transition.