UXIT-1415 - Rework Image Logic #648
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
mirhamasala
requested review from
CharlyMartin
and removed request for
mirhamasala
|
|
||
| const { src: fallbackSrc, alt: fallbackAlt } = graphicsData.imageFallback | ||
|
|
||
| async function getImageSrc(src: string | StaticImport) { |
There was a problem hiding this comment.
| async function getImageSrc(src: string | StaticImport) { | |
| async function getImageSrc(src: StaticImport["src"]) { |
| const { src: fallbackSrc, alt: fallbackAlt } = graphicsData.imageFallback | ||
|
|
||
| async function getImageSrc(src: string | StaticImport) { | ||
| const isStaticImport = typeof src === 'object' |
There was a problem hiding this comment.
Maybe we can rename isStaticImport to isLocalImage to be consistent with the types defined in imageTypes.ts?
There was a problem hiding this comment.
I would probably rename everything to use static or dynamic since those terms come straight from Next.js docs. Is there an example where this wouldn't work?
| switch (true) { | ||
| case isStaticImport: | ||
| return src | ||
| case noImage: | ||
| return fallbackSrc | ||
| case isRemoteImage: | ||
| const remoteImageExists = await checkRemoteImageExists(src) | ||
| return remoteImageExists ? src : fallbackSrc | ||
| default: | ||
| return src | ||
| } |
There was a problem hiding this comment.
It took me a little while to understand the switch (true) statement, I had never seen this pattern before. And probably because I'm just back from holidays 😅
Just curious, is there a benefit to this over if statements? Or is it a personal preference?
| async function getImageSrc(src: string | StaticImport) { | ||
| const isStaticImport = typeof src === 'object' | ||
| const isRemoteImage = | ||
| typeof src === 'string' && (src.startsWith('http') || src.startsWith('//')) |
There was a problem hiding this comment.
What do you think about doing typeof src === 'string' && src !== "" instead?
| async function getImageSrc(src: string | StaticImport) { | ||
| const isStaticImport = typeof src === 'object' | ||
| const isRemoteImage = | ||
| typeof src === 'string' && (src.startsWith('http') || src.startsWith('//')) | ||
| const noImage = !src | ||
|
|
||
| switch (true) { | ||
| case isStaticImport: | ||
| return src | ||
| case noImage: | ||
| return fallbackSrc | ||
| case isRemoteImage: | ||
| const remoteImageExists = await checkRemoteImageExists(src) | ||
| return remoteImageExists ? src : fallbackSrc | ||
| default: | ||
| return src | ||
| } | ||
| } | ||
|
|
||
| async function checkRemoteImageExists(url: string) { | ||
| try { | ||
| const response = await fetch(url, { method: 'HEAD' }) | ||
| return response.ok | ||
| } catch (error) { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we move getImageSrc and checkRemoteImageExists below ImageWithFallback?
| <ImageWithFallback | ||
| fill | ||
| alt={alt} | ||
| src={src} | ||
| blurDataURL={blurDataURL} | ||
| style={{ objectFit: 'cover' }} | ||
| className="h-full w-full rounded-lg" | ||
| sizes={buildImageSizeProp({ | ||
| startSize: '100vw', |
There was a problem hiding this comment.
Same comment as before - the prop definition suggests a local image, but the implementation suggests a remote image. Maybe we can use Next's Image component here?
| <ImageWithFallback | ||
| src={src} | ||
| height={height} | ||
| width={width} | ||
| blurDataURL={blurDataURL} |
There was a problem hiding this comment.
Same comment as above.
| <ImageWithFallback | ||
| src={src} | ||
| alt={alt} | ||
| height={height} | ||
| width={width} | ||
| blurDataURL={blurDataURL} |
There was a problem hiding this comment.
Same comment as above.
| @@ -36,6 +36,8 @@ export function FeaturedBlogPosts() { | |||
| category, | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
The import of graphicsData can be deleted at the top of the file.
| /> | ||
| ))} | ||
| {ecosystemProjects.map(({ slug, title, description, image }) => { | ||
| const { src } = image || {} |
There was a problem hiding this comment.
The import of graphicsData can be deleted at the top of the file.
|
First off, this implementation is a readability improvement over the initial one, so good one! 🙂 I’ve left some comments, but my main question is about // src/app/_components/ImageWithFallback.tsx
const isStaticImport = typeof src === 'object'From what I understand, local images “never” fail to load because they’re imported at build time. If there’s an issue with the import, the build itself would fail. Because of this, I’m tempted to suggest that This split also aligns with the (subtle) differences in handling local vs. remote images in Next.js. Typically, local images don’t need a wrapper container since they come with predefined I understand that this approach means they are two image components, which might not be what we want. I don't see it as a problem, but maybe it will be confusing in the long run, Not sure, let me know what you think :) |
| <div className="relative aspect-video"> | ||
| <DynamicImage | ||
| <ImageWithFallback |
There was a problem hiding this comment.
Why do we need to name it explicitly WithFallback? I kinda expect a fallback out of the box?
|
|
||
| type ArticleHeaderProps = { | ||
| image?: ImageProps | ||
| image?: RemoteImageData | LocalImageData |
There was a problem hiding this comment.
What's the reason that this is both types?
| alt: '', | ||
| ...(image || { | ||
| ...graphicsData.imageFallback, | ||
| }), | ||
| fallback: graphicsData.imageFallback, | ||
| src: src || '', |
There was a problem hiding this comment.
I was hoping we wouldn't have to use generic fallbacks to '' anymore...
| /> | ||
| ))} | ||
| {ecosystemProjects.map(({ slug, title, description, image }) => { | ||
| const { src } = image || {} |
| /> | ||
| ))} | ||
| {grantGraduates.map(({ title, description, slug, image }) => { | ||
| const { src } = image || {} |
|
@barbaraperic @CharlyMartin I’m still feeling a bit unclear about this implementation. It’s hard to distinguish how images are being used, specifically what parts are static and what parts are dynamic. My hope was that we could first deconstruct everything, use Next.js’ Image component consistently throughout, and then, if necessary, start refactoring from there (as outlined in the spec). |
📝 Description
This PR refactors the code to use the native Next image directly, replacing the
StaticImageandDynamicImagecomponents.🛠️ Key Changes
StaticImageandDynamicImagecomponent and replaced with Next'sImage📌 To-Do Before Merging
🧪 How to Test