react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Loading attribute on Image Component

Open mrtzdev opened this issue 1 year ago โ€ข 6 comments

๐Ÿ™‹ Feature Request

Support for loading attribute on Image Component (@react-spectrum/image) via a prop: loading.

The loading attribute allows a browser to defer loading offscreen images until users scroll near them: https://web.dev/browser-level-image-lazy-loading/ https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes

๐Ÿค” Expected Behavior

It should be possible to add a prop "loading" on the Image Component like this:

<Image src="https://i.imgur.com/Z7AzH2c.png" loading="lazy" alt="Sky and roof" />

Type: loading?: "eager" | "lazy"

๐Ÿ˜ฏ Current Behavior

Currently it is not possible to add a loading prop to the Image Component. It is not rendered on the img element.

๐Ÿ’ Possible Solution

The loading attribute could be added like this:

@react-spectrum/image/src/Image.tsx https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/image/src/Image.tsx


import {classNames, useDOMRef, useSlotProps, useStyleProps} from '@react-spectrum/utils';
import {DOMRef} from '@react-types/shared';
import {filterDOMProps} from '@react-aria/utils';
import React from 'react';
import {SpectrumImageProps} from '@react-types/image';
import styles from '@adobe/spectrum-css-temp/components/image/vars.css';
import {useProviderProps} from '@react-spectrum/provider';



function Image(props: SpectrumImageProps, ref: DOMRef<HTMLDivElement>) {
  /* Slots should be able to pass an alt for default behavior, but in Images, the child may know better. */
  let userProvidedAlt = props.alt;
  props = useSlotProps(props, 'image');
  props = useProviderProps(props);
  let {
    objectFit,
    src,
    alt,
    loading,
    ...otherProps
  } = props;
  let {styleProps} = useStyleProps(otherProps);
  let domRef = useDOMRef(ref);

  if (alt == null) {
    console.warn(
      'The `alt` prop was not provided to an image. ' +
      'Add `alt` text for screen readers, or set `alt=""` prop to indicate that the image ' +
      'is decorative or redundant with displayed text and should not be announced by screen readers.'
    );
  }

  return (
    <div
      {...filterDOMProps(props)}
      {...styleProps}
      className={classNames(styles, styleProps.className)}
      style={{
        ...styleProps.style,
        overflow: 'hidden'
      }}
      ref={domRef}>
      <img
        src={src}
        alt={userProvidedAlt || alt}
        loading={loading}
        style={{objectFit}}
        className={classNames(styles, 'spectrum-Image-img')} />
    </div>
  );
}

/**
 * Image is used to insert and display an image within a component.
 */
const _Image = React.forwardRef(Image);
export {_Image as Image};


and in @react-types/image: https://github.com/adobe/react-spectrum/blob/main/packages/%40react-types/image/src/index.d.ts


import {DOMProps, StyleProps} from '@react-types/shared';

export interface ImageProps {
  /**
   * The URL of the image.
   */
  src: string,
  /**
   * Text description of the image.
   */
  alt?: string,
  /**
   * Sets the Image [object-fit](https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit) style.
   */
  objectFit?: any, // move to styleProps for images and type better

   /**
   * Sets the Image lazy loading attribute (https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/loading).
   */
 
  loading?: "eager" | "lazy"
}

export interface SpectrumImageProps extends ImageProps, DOMProps, StyleProps {
  /**
   * A slot to place the image in.
   * @default 'image'
   */
  slot?: string
}


Good idea ? Can I open a PR for this ?

๐Ÿ”ฆ Context

๐Ÿ’ป Examples

๐Ÿงข Your Company/Team

๐ŸŽ Tracking Ticket (optional)

mrtzdev avatar Aug 14 '22 18:08 mrtzdev

This seems reasonable. Not sure if there are any others we would want to include, short of extending https://github.com/microsoft/TypeScript/blob/9767f51758c2899ad09c3466fd6ebbde79f53f63/lib/lib.dom.d.ts#L6925

reidbarber avatar Aug 16 '22 15:08 reidbarber

ok, i think the following attributes are also useful and important:
srcset and sizes ( for responsive images ) width and height ( to optimize Cumulative Layout Shift (cls) and prevent layout shifts ) decoding ( https://html.spec.whatwg.org/multipage/images.html#image-decoding-hint )

In my use case, i also need a onload event with a callback. onLoad onError props ?

We should also include a complete example of a responsive image with srcset etc. in the docs.

mrtzdev avatar Aug 19 '22 12:08 mrtzdev

@mrtzdev Mind sharing what you are using Image for? Could you use a img element instead?

LFDanLu avatar Aug 24 '22 18:08 LFDanLu

@LFDanLu I could use the img element instead and build my own image component.. But I think the attributes are really useful for a image component like this:

Other Examples: chakra ui image component: https://chakra-ui.com/docs/components/image/props

pinterest gestalt ui: https://gestalt.pinterest.systems/web/image

next/image https://nextjs.org/docs/api-reference/next/image

What is the downside if we add it ?

mrtzdev avatar Aug 31 '22 13:08 mrtzdev

The team is a bit wary of overloading the Image component since at its core it intended for use within other React Spectrum components (hence why useSlotProps exists within it) rather than being a straight up replacement for the img element everywhere. That being said, we aren't dismissing the possibility of adding the loading attribute or other attributes to it, we just haven't run into a use case within our components that needed it or any other extra attributes.

For now I'd recommend that you use the img element if at all possible and we can keep this issue open since we maybe introducing loading when Cards/CardView gets further along.

LFDanLu avatar Aug 31 '22 16:08 LFDanLu

okay that make sense. Thanks for your reply.

mrtzdev avatar Sep 01 '22 06:09 mrtzdev