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.

Sign up for GitHub

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

[UXIT-1731] Validate Image Src #919

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

[UXIT-1731] Validate Image Src #919

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5575a46
Create image component that handles fallback is src is invalid
barbaraperic Dec 16, 2024
4d1f3e7
Fix imports
barbaraperic Dec 16, 2024
43d75d8
Use use server for Smart Image component
barbaraperic Dec 16, 2024
9b79ada
Fix required alt prop
barbaraperic Dec 16, 2024
3176bb6
Remove the return type
barbaraperic Dec 16, 2024
667e4c3
Merge branch 'main' into bp/validate-image-src
barbaraperic Dec 19, 2024
15b548b
fix: correct image source handling in Card component
barbaraperic Dec 19, 2024
60c78d2
- Replace SmartImage with Next.js Image component in Card and PageHeader
barbaraperic Dec 19, 2024
c5543cd
Rollback to where alt is possibly undefined in SmartImage
barbaraperic Dec 19, 2024
3012340
fix: add fallback for empty alt text in Card Image component
barbaraperic Dec 19, 2024
ba2300c
fix: add fallback for empty alt text in PageHeader Image component
barbaraperic Dec 19, 2024
2c75747
Merge branch 'main' into bp/validate-image-src
barbaraperic Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/app/_components/ArticleHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import Image from 'next/image'

import type { ImageProps } from '@/types/imageType'

import { buildImageSizeProp } from '@/utils/buildImageSizeProp'

import { type HeadingProps, Heading } from '@/components/Heading'
import { SmartImage, type SmartImageProps } from '@/components/SmartImage'

type ArticleHeaderProps = {
image: ImageProps
image: SmartImageProps
children?: React.ReactNode
}

Expand All @@ -20,7 +17,7 @@ export function ArticleHeader({ image, children }: ArticleHeaderProps) {
<header className="space-y-6">
<div className="space-y-6">{children}</div>
<div className="relative aspect-video">
<Image
<SmartImage
fill
priority
quality={100}
Expand Down
19 changes: 6 additions & 13 deletions src/app/_components/Card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import Image, { type ImageProps } from 'next/image'
import { type StaticImageData } from 'next/image'

import { ArrowUpRight } from '@phosphor-icons/react/dist/ssr'
import { clsx } from 'clsx'
import theme from 'tailwindcss/defaultTheme'

import { type CTAProps } from '@/types/ctaType'
import type { ImageObjectFit, StaticImageProps } from '@/types/imageType'

import { buildImageSizeProp } from '@/utils/buildImageSizeProp'
import { isExternalLink } from '@/utils/linkUtils'
Expand All @@ -15,22 +14,16 @@ import { BaseLink } from '@/components/BaseLink'
import { Heading } from '@/components/Heading'
import { Icon } from '@/components/Icon'
import { Meta, type MetaDataType } from '@/components/Meta'
import { SmartImage, type SmartImageProps } from '@/components/SmartImage'
import { type TagGroupProps, TagGroup } from '@/components/TagGroup'

type CardImageProps = (StaticImageProps | ImageProps) & {
objectFit?: ImageObjectFit
padding?: boolean
priority?: boolean
sizes?: string
}

type CardProps = {
title: string | React.ReactNode
tagLabel?: TagGroupProps['label']
metaData?: MetaDataType
description?: string
cta?: CTAPropsWithSpacing
image?: CardImageProps
image?: SmartImageProps
borderColor?: 'brand-300' | 'brand-400' | 'brand-500' | 'brand-600'
textIsClamped?: boolean
as?: React.ElementType
Expand Down Expand Up @@ -114,18 +107,18 @@ Card.Image = function ImageComponent({

if (isStaticImage) {
return (
<Image
<SmartImage
{...commonProps}
className={clsx(commonProps.className, 'aspect-video')}
src={image.data}
src={(image.data as StaticImageData).src}
alt={commonProps.alt}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript is complaining here although we have a isStaticImage condition (which check data in the image)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should just be image.src no? image.data doesn't exist on SmartImageProps

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry not sure I understand 😕 StaticImages have data key, like this:

{
  data: {
    src: '/_next/static/media/022624-ff-anualreport.b5deae8f.png',
    height: 1536,
    width: 3072,
    blurDataURL: '/_next/image?url=%2F_next%2Fstatic%2Fmedia%2F022624-ff-anualreport.b5deae8f.png&w=8&q=70',
    blurWidth: 8,
    blurHeight: 4
  },
  alt: '',
  sizes: '(min-width: 640px) 710px, (min-width: 768px) 980px, (min-width: 1024px) 480px, 100vw'
}

Ah so this should actually be image.data...

/>
)
}

return (
<div className="relative aspect-video">
<Image
<SmartImage
fill
{...commonProps}
className={clsx(commonProps.className, 'h-full w-full')}
Expand Down
2 changes: 1 addition & 1 deletion src/app/_components/FeaturedBlogPosts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export function FeaturedBlogPosts() {
text: 'Learn More',
}}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
objectFit: 'cover',
sizes: buildImageSizeProp({
Expand Down
2 changes: 1 addition & 1 deletion src/app/_components/FeaturedEcosystemProjects.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function FeaturedEcosystemProjects({
icon: MagnifyingGlass,
}}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
objectFit: 'contain',
padding: Boolean(image),
Expand Down
2 changes: 1 addition & 1 deletion src/app/_components/FeaturedGrantGraduates.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function FeaturedGrantsGraduates({
icon: BookOpen,
}}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
objectFit: 'contain',
padding: Boolean(image),
Expand Down
17 changes: 6 additions & 11 deletions src/app/_components/PageHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import Image, { type ImageProps } from 'next/image'
import type { StaticImageData } from 'next/image'

import { clsx } from 'clsx'

import type { ImageObjectFit, StaticImageProps } from '@/types/imageType'

import { buildImageSizeProp } from '@/utils/buildImageSizeProp'

import {
Expand All @@ -17,18 +15,15 @@ import {
import { Heading } from '@/components/Heading'
import { Meta, type MetaDataType } from '@/components/Meta'
import { SectionDivider } from '@/components/SectionDivider'
import { SmartImage, type SmartImageProps } from '@/components/SmartImage'

type TitleProps = {
children: string
}

type PageHeaderImageProps = (StaticImageProps | ImageProps) & {
objectFit?: ImageObjectFit
}

type PageHeaderProps = {
title: TitleProps['children']
image: PageHeaderImageProps
image: SmartImageProps
isFeatured?: boolean
metaData?: MetaDataType
description?: DescriptionTextType
Expand Down Expand Up @@ -90,18 +85,18 @@ PageHeader.Image = function PageHeaderImage({

if (isStaticImage) {
return (
<Image
<SmartImage
{...commonProps}
className={clsx(commonProps.className, 'aspect-video')}
src={image.data}
src={image.data as StaticImageData}
alt={commonProps.alt}
/>
)
}

return (
<div className="relative aspect-video">
<Image
<SmartImage
fill
{...commonProps}
className={clsx(commonProps.className, 'h-full w-full')}
Expand Down
86 changes: 86 additions & 0 deletions src/app/_components/SmartImage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
'use server'
Copy link
Collaborator Author

@barbaraperic barbaraperic Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to be explicit to run on the server because fs and path exclusively run in the node env

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't components 'use server' by default?

I haven't seen this used in components. What does it do?

Copy link
Collaborator Author

@barbaraperic barbaraperic Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically makes sure it runs server side. For example, in GovernanceCalendarCard (use client component) we import Card component which uses SmartImage. if we don't explicitly write use server in the SmartImage component, it will run on the client, and in that case we get an error because we use fs which can't run in the browser.


import fs from 'fs/promises'
import path from 'path'

import { type ImageProps } from 'next/image'
import Image from 'next/image'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Group imports?


import type { ImageObjectFit, StaticImageProps } from '@/types/imageType'

import { graphicsData } from '@/data/graphicsData'

const { data: fallbackSrc, alt: fallbackAlt } = graphicsData.imageFallback

export type SmartImageProps = {
src?: string | StaticImageProps['data']
alt?: string
className?: string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

className?: string can be included in Omit<ImageProps, 'src' | 'alt'>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm it's included in the line 21? Or you're referring to something else?

objectFit?: ImageObjectFit
padding?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does padding do?

Copy link
Collaborator Author

@barbaraperic barbaraperic Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use it in the Card component, same as objectFit

} & Omit<ImageProps, 'src' | 'alt' | 'className'>

export async function SmartImage({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about something like this?

Does it read a bit better to you?

export async function SmartImage({ src, alt = '', ...props }: SmartImageProps) {
  if (!src) {
    return <Image src={fallbackSrc} alt={fallbackAlt} {...props} />
  }

  const isValid = await checkImageValidity(src)

  return (
    <Image
      src={isValid ? src : fallbackSrc}
      alt={isValid ? alt : fallbackAlt}
      {...props}
    />
  )
}

async function checkImageValidity(src: NonNullable<SmartImageProps['src']>) {
  const isStaticImport = typeof src === 'object'
  const isAssetImage = typeof src === 'string' && src.startsWith('/assets')

  if (isStaticImport) {
    return true
  }

  if (isAssetImage) {
    return await checkAssetImageExists(src)
  }

  return await checkRemoteImageExists(src)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, personally I kind of like the separation of logic (getImageSrc) and ui rendering (SmartImage). That said, I’m happy to explore versions if you think this doesn’t work well for the current context.

src,
alt = '',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we make alt an empty string if undefined? I don't understand 🙃

className,
...props
}: SmartImageProps) {
const imageSrc = await getImageSrc(src)

const imageExists = imageSrc !== fallbackSrc

return (
<Image
className={className}
src={imageSrc}
alt={imageExists ? alt : fallbackAlt}
{...props}
/>
)
}

async function getImageSrc(src: SmartImageProps['src']) {
if (!src) {
return fallbackSrc
}
const isStaticImport = typeof src === 'object'
const isAssetImage = typeof src === 'string' && src.startsWith('/assets')
const isRemoteImage =
typeof src === 'string' && (src.startsWith('http') || src.startsWith('//'))

if (isStaticImport) {
return src
}

if (isAssetImage) {
const assetExists = await checkAssetImageExists(src)
return assetExists ? src : fallbackSrc
}

if (isRemoteImage) {
const remoteImageExists = await checkRemoteImageExists(src)
return remoteImageExists ? src : fallbackSrc
}

return src
}

async function checkRemoteImageExists(url: string) {
try {
const response = await fetch(url, { method: 'HEAD' })
return response.ok
} catch {
return false
}
}

async function checkAssetImageExists(src: string): Promise<boolean> {
const publicPath = path.join(process.cwd(), 'public', src)
try {
await fs.access(publicPath, fs.constants.F_OK)
return true
} catch {
return false
}
}
2 changes: 1 addition & 1 deletion src/app/about/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export default function About() {
description={description}
image={
image && {
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
sizes: buildImageSizeProp({
startSize: '100vw',
Expand Down
2 changes: 1 addition & 1 deletion src/app/blog/[slug]/components/BlogPostHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export function BlogPostHeader({
return (
<ArticleHeader
image={{
src: image?.src || graphicsData.imageFallback.data.src,
src: image?.src,
alt: '',
}}
>
Expand Down
4 changes: 2 additions & 2 deletions src/app/blog/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export default function Blog({ searchParams }: Props) {
description={featuredPost.description}
metaData={getMetaData(featuredPost.publishedOn)}
image={{
...(featuredPost.image || graphicsData.imageFallback.data),
...featuredPost.image,
alt: '',
objectFit: 'cover',
}}
Expand Down Expand Up @@ -196,7 +196,7 @@ export default function Blog({ searchParams }: Props) {
icon: BookOpen,
}}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
priority: isFirstTwoImages,
objectFit: 'cover',
Expand Down
4 changes: 1 addition & 3 deletions src/app/digest/[slug]/components/DigestArticleHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { graphicsData } from '@/data/graphicsData'

import { ArticleHeader } from '@/components/ArticleHeader'
import { AvatarGroup } from '@/components/AvatarGroup'
import { TagGroup } from '@/components/TagGroup'
Expand All @@ -21,7 +19,7 @@ export function DigestArticleHeader({
return (
<ArticleHeader
image={{
src: image?.src || graphicsData.imageFallback.data.src,
src: image?.src,
alt: '',
}}
>
Expand Down
2 changes: 1 addition & 1 deletion src/app/digest/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export default function Digest() {
icon: BookOpen,
}}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: image?.alt || '',
sizes: buildImageSizeProp({
startSize: '100vw',
Expand Down
14 changes: 4 additions & 10 deletions src/app/ecosystem-explorer/[slug]/components/PageHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,21 @@
import Image, { type ImageProps } from 'next/image'

import { clsx } from 'clsx'

import { graphicsData } from '@/data/graphicsData'

import { buildImageSizeProp } from '@/utils/buildImageSizeProp'

type ImagePropsWithOptionalAlt = Omit<ImageProps, 'alt'> & {
alt?: string
}
import { SmartImage, type SmartImageProps } from '@/components/SmartImage'

type PageHeaderProps = {
image?: ImagePropsWithOptionalAlt
image?: SmartImageProps
}

export function PageHeader({ image }: PageHeaderProps) {
return (
<header className="space-y-10 md:space-y-16">
<div className="relative h-48 md:w-3/4 lg:w-2/3 xl:w-3/5">
<Image
<SmartImage
fill
priority
src={image?.src || graphicsData.imageFallback.data.src}
src={image?.src}
alt={''}
className={clsx(
image?.src && 'rounded-lg',
Expand Down
2 changes: 1 addition & 1 deletion src/app/ecosystem-explorer/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ export default function EcosystemExplorer({ searchParams }: Props) {
icon: BookOpen,
}}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
objectFit: 'contain',
padding: Boolean(image),
Expand Down
2 changes: 1 addition & 1 deletion src/app/events/[slug]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export default function EventEntry({ params }: EventProps) {
eventHasConcluded,
})}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
objectFit: 'cover',
}}
Expand Down
4 changes: 2 additions & 2 deletions src/app/events/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export default function Events({ searchParams }: Props) {
location: featuredEvent.location.primary,
})}
image={{
...(featuredEvent.image || graphicsData.imageFallback.data),
...featuredEvent.image,
alt: '',
objectFit: 'cover',
}}
Expand Down Expand Up @@ -203,7 +203,7 @@ export default function Events({ searchParams }: Props) {
location: location.primary,
})}
image={{
...(image || graphicsData.imageFallback.data),
...image,
alt: '',
priority: isFirstTwoImages,
objectFit: 'cover',
Expand Down
Loading