adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.98k stars 1.13k forks source link

Loading attribute on Image Component #3404

Open mrtzdev opened 2 years ago

mrtzdev commented 2 years ago

πŸ™‹ 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)

reidbarber commented 2 years ago

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

mrtzdev commented 2 years ago

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.

LFDanLu commented 2 years ago

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

mrtzdev commented 2 years ago

@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 ?

LFDanLu commented 2 years ago

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.

mrtzdev commented 2 years ago

okay that make sense. Thanks for your reply.