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

maxWidth does not correctly set the width/height properties when the breakpoint is not met #72

Open
lifeiscontent opened this issue May 22, 2023 · 1 comment

Comments

@lifeiscontent
Copy link

Describe the bug

given this example: https://github.com/Josh-McFarlin/remix-image/blob/master/examples/basic/app/routes/index.tsx#L25-L38

I'd expect the width/height of the image to change depending on if the breakpoint was met, currently it only sets the size of the image to the image size that contains the maxWidth property. tbh all of the editing of CSS styles would be a non-issue if something like the picture element existed. imo adding these additional styles seems like more work than what I would expect this component to do.

Your Example Website or App

No response

Steps to Reproduce the Bug or Issue

  1. Goto this example: https://github.com/Josh-McFarlin/remix-image/blob/master/examples/basic/app/routes/index.tsx#L25-L38
  2. see that the width of the image is hard-coded with a style property to be the width of what is set in the maxWidth variant of the responsive property.

Expected behavior

I think setting width/height properties in this component seems a little overblown in terms of an expectation of what it should be doing for me, I would expect to define my own width/height classes in my own code.

Screenshots or Videos

No response

Platform

  • OS: MacOS
  • Browser: Chrome
  • Version: 113.0.5672.126

Additional context

No response

@lifeiscontent
Copy link
Author

lifeiscontent commented May 22, 2023

this is what I came up with:

Context

I made the placeholder URL a CSS variable so it could be easily accessed in CSS and styled with something like tailwind fairly easily.

image.tsx

/* eslint-disable jsx-a11y/alt-text */
import { forwardRef, memo, useCallback, useState } from 'react'
import type { ImageProps } from 'remix-image'
import { remixImageLoader, useResponsiveImage } from 'remix-image'
import { useForkRef } from '~/hooks/useForkRef'

export const Image = memo<ImageProps>(
	forwardRef<HTMLImageElement, ImageProps>(
		(
			{
				src,
				loaderUrl = '/api/image',
				loader = remixImageLoader,
				responsive = [],
				options = {},
				dprVariants = 1,
				decoding = 'async',
				loading = 'lazy',
				unoptimized = false,
				placeholder = 'empty',
				blurDataURL = !unoptimized && placeholder === 'blur'
					? loader(src ?? '', loaderUrl, {
							width: 15,
							height: 15,
							fit: options?.fit ?? 'cover',
							position: options?.position ?? 'center',
							contentType: options.contentType,
					  })
					: null,
				style,
				...imgProps
			}: ImageProps,
			ref,
		) => {
			const [loaded, setLoaded] = useState(false)
			const responsiveProps = useResponsiveImage(
				{ src },
				responsive,
				options,
				dprVariants,
				loaderUrl,
				loader,
			)
			const imgRef = useCallback<React.RefCallback<HTMLImageElement>>(ref => {
				if (ref) {
					if (ref.complete) {
						const p = ref ? ref.decode() : Promise.resolve()
						p.catch(() => {}).then(() => {
							setLoaded(true)
						})
					}
				}
			}, [])

			const handleRef = useForkRef(ref, imgRef)

			return (
				<img
					ref={handleRef}
					decoding={decoding}
					loading={loading}
					style={
						unoptimized
							? style
							: ({
									'--remix-image-blur-data-url': `url(${blurDataURL})`,
									...style,
							  } as React.CSSProperties)
					}
					data-remix-image={unoptimized ? undefined : true}
					data-remix-image-loaded={unoptimized ? undefined : loaded}
					data-remix-image-placeholder={unoptimized ? undefined : placeholder}
					{...imgProps}
					{...responsiveProps}
					onLoad={event => {
						const p = event.currentTarget
							? event.currentTarget.decode()
							: Promise.resolve()
						p.catch(() => {}).then(() => {
							setLoaded(true)
						})
						if (imgProps.onLoad) {
							imgProps.onLoad(event)
						}
					}}
				/>
			)
		},
	),
)

Image.displayName = 'Image'

app-image.tsx

import { forwardRef, memo } from 'react'
import { Image } from '../Image/Image'
import { clsx } from 'clsx'

export const AppImage = memo(
	forwardRef<HTMLImageElement, React.ComponentPropsWithoutRef<typeof Image>>(
		({ className, ...props }, ref) => (
      <Image
        {...props}
        className={clsx(
          "data-[remix-image-placeholder='blur']:transition-all data-[remix-image-placeholder='blur']:will-change-transform data-[remix-image-placeholder='blur']:duration-1000 data-[remix-image-placeholder='blur']:data-[remix-image-loaded='false']:blur-lg data-[remix-image-placeholder='blur']:data-[remix-image-loaded='false']:bg-no-repeat data-[remix-image-placeholder='blur']:data-[remix-image-loaded='false']:bg-cover data-[remix-image-placeholder='blur']:data-[remix-image-loaded='false']:bg-center data-[remix-image-placeholder='blur']:data-[remix-image-loaded='false']:[background-image:var(--remix-image-blur-data-url)]",
          className,
        )}
        ref={ref}
      />
    ),
	),
)

AppImage.displayName = 'AppImage'

this then allows me to do the following:

			<AppImage
				alt={alt}
				className='w-[248px] h-[248px] lg:w-[268px] lg:h-[268px]'
				dprVariants={[1, 2]}
				height={248}
				loading="lazy"
				placeholder="blur"
				responsive={[
					{
						size: { width: 248, height: 248 },
						maxWidth: 1023,
					},
					{
						size: { width: 268, height: 268 },
					},
				]}
				src="/some-image.jpg"
				width={248}
			/>

for the sake of conversation about it, I also created a Picture/Source element that could be added.

import { memo, forwardRef } from 'react'

export const Picture = memo(
	forwardRef<HTMLPictureElement, React.ComponentPropsWithoutRef<'picture'>>(
		(props, ref) => <picture {...props} ref={ref} />,
	),
)

Picture.displayName = 'Picture'
import { forwardRef, memo } from 'react'
import type { ImageProps } from 'remix-image'
import { remixImageLoader, useResponsiveImage, MimeType } from 'remix-image'

function getContentType(
	type: string,
	defaultType?: MimeType,
): MimeType | undefined {
	switch (type) {
		case MimeType.SVG:
			return MimeType.SVG
		case MimeType.JPEG:
			return MimeType.JPEG
		case MimeType.PNG:
			return MimeType.PNG
		case MimeType.GIF:
			return MimeType.GIF
		case MimeType.WEBP:
			return MimeType.WEBP
		case MimeType.BMP:
			return MimeType.BMP
		case MimeType.TIFF:
			return MimeType.TIFF
		case MimeType.AVIF:
			return MimeType.AVIF
		default:
			return defaultType
	}
}

function getOptions(
	options: ImageProps['options'],
	props: React.ComponentPropsWithoutRef<'source'>,
): ImageProps['options'] {
	if (props.type) {
		return {
			...options,
			contentType: getContentType(props.type, options?.contentType),
		}
	}
	return options
}

export const Source = memo(
	forwardRef<
		HTMLSourceElement,
		React.ComponentPropsWithoutRef<'source'> &
			Pick<
				ImageProps,
				'loaderUrl' | 'loader' | 'responsive' | 'options' | 'dprVariants'
			>
	>(
		(
			{
				src,
				loaderUrl = '/api/image',
				loader = remixImageLoader,
				responsive = [],
				options = {},
				dprVariants = 1,
				...props
			},
			ref,
		) => {
			const responsiveProps = useResponsiveImage(
				{ src },
				responsive,
				getOptions(options, props),
				dprVariants,
				loaderUrl,
				loader,
			)

			return (
				<source
					{...props}
					{...responsiveProps}
					src={
						props.type
							? loader(src || '', loaderUrl, {
									width: props.width
										? parseInt(props.width.toString(), 10)
										: undefined,
									height: props.height
										? parseInt(props.height.toString(), 10)
										: undefined,
									fit: 'cover',
									position: 'center',
									background: [0x00, 0x00, 0x00, 0x00],
									quality: 80,
									compressionLevel: 9,
									loop: 0,
									delay: 100,
									blurRadius: null,
									rotate: null,
									flip: null,
									crop: null,
									...options,
							  })
							: responsiveProps.src
					}
					ref={ref}
				/>
			)
		},
	),
)

Source.displayName = 'Source'

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

No branches or pull requests

1 participant