feat: add responsive support to picture component (#12423)

* feat: add responsive support to picture component * Add picture tests * Fix test * Implement own define vars * Share logic * Prevent extra astro-* class * Use dataset scoping * Revert unit test * Revert "Fix test" This reverts commit f9ec6e2. * Changes from review
withastro · Nov 14, 2024 · f752102 · f752102
1 parent 582f997
commit f752102
Show file tree

Hide file tree

Showing 10 changed files with 400 additions and 77 deletions.
diff --git a/packages/astro/components/Image.astro b/packages/astro/components/Image.astro
@@ -1,7 +1,10 @@
 ---
 import { type LocalImageProps, type RemoteImageProps, getImage, imageConfig } from 'astro:assets';
+import type { UnresolvedImageTransform } from '../dist/assets/types';
+import { applyResponsiveAttributes } from '../dist/assets/utils/imageAttributes.js';
 import { AstroError, AstroErrorData } from '../dist/core/errors/index.js';
 import type { HTMLAttributes } from '../types';
+import './image.css';
 
 // The TypeScript diagnostic for JSX props uses the last member of the union to suggest props, so it would be better for
 // LocalImageProps to be last. Unfortunately, when we do this the error messages that remote images get are complete nonsense
@@ -23,21 +26,17 @@ if (typeof props.height === 'string') {
 	props.height = parseInt(props.height);
 }
 
-const { experimentalResponsiveImages } = imageConfig;
+const layout = props.layout ?? imageConfig.experimentalLayout ?? 'none';
+const useResponsive = imageConfig.experimentalResponsiveImages && layout !== 'none';
 
-const layoutClassMap = {
-	fixed: 'aim-fi',
-	responsive: 'aim-re',
-};
-
-if (experimentalResponsiveImages) {
+if (useResponsive) {
 	// Apply defaults from imageConfig if not provided
 	props.layout ??= imageConfig.experimentalLayout;
 	props.fit ??= imageConfig.experimentalObjectFit ?? 'cover';
 	props.position ??= imageConfig.experimentalObjectPosition ?? 'center';
 }
 
-const image = await getImage(props);
+const image = await getImage(props as UnresolvedImageTransform);
 
 const additionalAttributes: HTMLAttributes<'img'> = {};
 if (image.srcSet.values.length > 0) {
@@ -48,51 +47,15 @@ if (import.meta.env.DEV) {
 	additionalAttributes['data-image-component'] = 'true';
 }
 
-const cssFitValues = ['fill', 'contain', 'cover', 'scale-down'];
-
-// The style prop can't be spread when using define:vars, so we need to extract it here
-// @see https://github.com/withastro/compiler/issues/1050
-const { style = '', class: className, ...attrs } = { ...additionalAttributes, ...image.attributes };
+const attributes = useResponsive
+	? applyResponsiveAttributes({
+			layout,
+			image,
+			props,
+			additionalAttributes,
+		})
+	: { ...additionalAttributes, ...image.attributes };
 ---
 
-{
-	experimentalResponsiveImages && props.layout ? (
-		<img
-			src={image.src}
-			{...attrs}
-			{style}
-			class={`${layoutClassMap[props.layout ?? imageConfig.experimentalLayout] ?? ''} aim ${className ?? ''}`.trim()}
-		/>
-	) : (
-		<img src={image.src} {...additionalAttributes} {...image.attributes} class={className} />
-	)
-}
-
-<style
-	define:vars={experimentalResponsiveImages &&
-		props.layout && {
-			w: image.attributes.width ?? props.width ?? image.options.width,
-			h: image.attributes.height ?? props.height ?? image.options.height,
-			fit: cssFitValues.includes(props.fit) && props.fit,
-			pos: props.position,
-		}}
->
-	/* Shared by all Astro images */
-	.aim {
-		width: 100%;
-		height: auto;
-		object-fit: var(--fit);
-		object-position: var(--pos);
-		aspect-ratio: var(--w) / var(--h);
-	}
-	/* Styles for responsive layout */
-	.aim-re {
-		max-width: calc(var(--w) * 1px);
-		max-height: calc(var(--h) * 1px);
-	}
-	/* Styles for fixed layout */
-	.aim-fi {
-		width: calc(var(--w) * 1px);
-		height: calc(var(--h) * 1px);
-	}
-</style>
+{/* Applying class outside of the spread prevents it from applying unnecessary astro-* classes */}
+<img src={image.src} {...attributes} class={attributes.class} />
diff --git a/packages/astro/components/Picture.astro b/packages/astro/components/Picture.astro
@@ -1,10 +1,16 @@
 ---
-import { type LocalImageProps, type RemoteImageProps, getImage } from 'astro:assets';
+import { type LocalImageProps, type RemoteImageProps, getImage, imageConfig } from 'astro:assets';
 import * as mime from 'mrmime';
+import { applyResponsiveAttributes } from '../dist/assets/utils/imageAttributes';
 import { isESMImportedImage, resolveSrc } from '../dist/assets/utils/imageKind';
 import { AstroError, AstroErrorData } from '../dist/core/errors/index.js';
-import type { GetImageResult, ImageOutputFormat } from '../dist/types/public/index.js';
+import type {
+	GetImageResult,
+	ImageOutputFormat,
+	UnresolvedImageTransform,
+} from '../dist/types/public/index.js';
 import type { HTMLAttributes } from '../types';
+import './image.css';
 
 type Props = (LocalImageProps | RemoteImageProps) & {
 	formats?: ImageOutputFormat[];
@@ -37,6 +43,17 @@ if (scopedStyleClass) {
 		pictureAttributes.class = scopedStyleClass;
 	}
 }
+
+const layout = props.layout ?? imageConfig.experimentalLayout ?? 'none';
+const useResponsive = imageConfig.experimentalResponsiveImages && layout !== 'none';
+
+if (useResponsive) {
+	// Apply defaults from imageConfig if not provided
+	props.layout ??= imageConfig.experimentalLayout;
+	props.fit ??= imageConfig.experimentalObjectFit ?? 'cover';
+	props.position ??= imageConfig.experimentalObjectPosition ?? 'center';
+}
+
 for (const key in props) {
 	if (key.startsWith('data-astro-cid')) {
 		pictureAttributes[key] = props[key];
@@ -53,7 +70,7 @@ const optimizedImages: GetImageResult[] = await Promise.all(
 				format: format,
 				widths: props.widths,
 				densities: props.densities,
-			}),
+			} as UnresolvedImageTransform),
 	),
 );
 
@@ -71,7 +88,7 @@ const fallbackImage = await getImage({
 	format: resultFallbackFormat,
 	widths: props.widths,
 	densities: props.densities,
-});
+} as UnresolvedImageTransform);
 
 const imgAdditionalAttributes: HTMLAttributes<'img'> = {};
 const sourceAdditionalAttributes: HTMLAttributes<'source'> = {};
@@ -85,6 +102,15 @@ if (fallbackImage.srcSet.values.length > 0) {
 	imgAdditionalAttributes.srcset = fallbackImage.srcSet.attribute;
 }
 
+const attributes = useResponsive
+	? applyResponsiveAttributes({
+			layout,
+			image: fallbackImage,
+			props,
+			additionalAttributes: imgAdditionalAttributes,
+		})
+	: { ...imgAdditionalAttributes, ...fallbackImage.attributes };
+
 if (import.meta.env.DEV) {
 	imgAdditionalAttributes['data-image-component'] = 'true';
 }
@@ -94,7 +120,7 @@ if (import.meta.env.DEV) {
 	{
 		Object.entries(optimizedImages).map(([_, image]) => {
 			const srcsetAttribute =
-				props.densities || (!props.densities && !props.widths)
+				props.densities || (!props.densities && !props.widths && !useResponsive)
 					? `${image.src}${image.srcSet.values.length > 0 ? ', ' + image.srcSet.attribute : ''}`
 					: image.srcSet.attribute;
 			return (
@@ -106,5 +132,6 @@ if (import.meta.env.DEV) {
 			);
 		})
 	}
-	<img src={fallbackImage.src} {...imgAdditionalAttributes} {...fallbackImage.attributes} />
+	{/* Applying class outside of the spread prevents it from applying unnecessary astro-* classes */}
+	<img src={fallbackImage.src} {...attributes} class={attributes.class} />
 </picture>
diff --git a/packages/astro/components/image.css b/packages/astro/components/image.css
@@ -0,0 +1,17 @@
+[data-astro-image] {
+	width: 100%;
+	height: auto;
+	object-fit: var(--fit);
+	object-position: var(--pos);
+	aspect-ratio: var(--w) / var(--h);
+}
+/* Styles for responsive layout */
+[data-astro-image='responsive'] {
+	max-width: calc(var(--w) * 1px);
+	max-height: calc(var(--h) * 1px);
+}
+/* Styles for fixed layout */
+[data-astro-image='fixed'] {
+	width: calc(var(--w) * 1px);
+	height: calc(var(--h) * 1px);
+}
diff --git a/packages/astro/src/assets/utils/imageAttributes.ts b/packages/astro/src/assets/utils/imageAttributes.ts
@@ -0,0 +1,49 @@
+import { toStyleString } from '../../runtime/server/render/util.js';
+import type { AstroConfig } from '../../types/public/config.js';
+import type { GetImageResult, ImageLayout, LocalImageProps, RemoteImageProps } from '../types.js';
+
+export function addCSSVarsToStyle(
+	vars: Record<string, string | false | undefined>,
+	styles?: string | Record<string, any>,
+) {
+	const cssVars = Object.entries(vars)
+		.filter(([_, value]) => value !== undefined && value !== false)
+		.map(([key, value]) => `--${key}: ${value};`)
+		.join(' ');
+
+	if (!styles) {
+		return cssVars;
+	}
+	const style = typeof styles === 'string' ? styles : toStyleString(styles);
+
+	return `${cssVars} ${style}`;
+}
+
+const cssFitValues = ['fill', 'contain', 'cover', 'scale-down'];
+
+export function applyResponsiveAttributes<
+	T extends LocalImageProps<unknown> | RemoteImageProps<unknown>,
+>({
+	layout,
+	image,
+	props,
+	additionalAttributes
+}: {
+	layout: Exclude<ImageLayout, 'none'>;
+	image: GetImageResult;
+	additionalAttributes: Record<string, any>;
+	props: T;
+}) {
+	const attributes = { ...additionalAttributes, ...image.attributes };
+	attributes.style = addCSSVarsToStyle(
+		{
+			w: image.attributes.width ?? props.width ?? image.options.width,
+			h: image.attributes.height ?? props.height ?? image.options.height,
+			fit: cssFitValues.includes(props.fit ?? '') && props.fit,
+			pos: props.position,
+		},
+		attributes.style,
+	);
+	attributes['data-astro-image'] = layout;
+	return attributes;
+}
diff --git a/packages/astro/src/runtime/server/render/util.ts b/packages/astro/src/runtime/server/render/util.ts
@@ -28,7 +28,8 @@ export const toAttributeString = (value: any, shouldEscape = true) =>
 
 const kebab = (k: string) =>
 	k.toLowerCase() === k ? k : k.replace(/[A-Z]/g, (match) => `-${match.toLowerCase()}`);
-const toStyleString = (obj: Record<string, any>) =>
+
+export const toStyleString = (obj: Record<string, any>) =>
 	Object.entries(obj)
 		.filter(([_, v]) => (typeof v === 'string' && v.trim()) || typeof v === 'number')
 		.map(([k, v]) => {