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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Image component #22470

Open
1 task done
oliviertassinari opened this issue Sep 3, 2020 · 13 comments
Open
1 task done

Add Image component #22470

oliviertassinari opened this issue Sep 3, 2020 · 13 comments
Labels
new feature New feature or request waiting for 馃憤 Waiting for upvotes

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 3, 2020

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 馃挕

Implement an enhanced version of the native <img> element.

Examples 馃寛

import { Image } from '@material-ui/core';

<Image src="" />

Motivation 馃敠

A couple of features we could bring:

  1. Use display: inline-block; to avoid layout jumps when the image is loading or broken.
  2. Provide a custom fallback component when the image fails to load.
  3. Support a loading indicator when the image takes too much time to load, e.g Skeleton.
  4. Add a smooth animation for displaying the image once loaded.
  5. Support a placeholder to display while image's loading. An image of lower resolution.
  6. Lazy loading? Maybe not relevant considering https://caniuse.com/#feat=loading-lazy-attr and https://web.dev/native-lazy-loading/.
  7. Support aspect ratio. For instance, it allows rendering an image with a width constraint without having to wait for the image to load over the networks to resolve its height (which might lead to a layout shift).

Benchmark

@oliviertassinari oliviertassinari added new feature New feature or request waiting for 馃憤 Waiting for upvotes labels Sep 3, 2020
@oliviertassinari oliviertassinari changed the title Add an Image component Add Image component Sep 3, 2020
@michael-land
Copy link
Contributor

michael-land commented Sep 5, 2020

<Image /> maybe not a good name for DX purpose if developers rely on editor autocomplete. because the Image class always be the first option.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/Image

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 5, 2020

<Image /> maybe not a good name for DX purpose if developers rely on editor autocomplete. because the Image class always be the first option.

@michael-land It reminds me of one of the reasons why we didn't call Typography: Text. In hindsight, it didn't seem to have stopped a lot of component libraries from using this name:

As long as you have eslint or TypeScript, it should be fine:

Capture d鈥檈虂cran 2020-09-05 a虁 17 14 13

Capture d鈥檈虂cran 2020-09-05 a虁 17 14 22

Otherwise, it's a nightmare:

Capture d鈥檈虂cran 2020-09-05 a虁 17 05 43


The other reason we have kept Typography was to match the theme.typography structure. But we could also argue that using two different names would reduce confusion when searching on Google 馃.


Next.js went with Image: https://nextjs.org/docs/app/api-reference/components/image

@eps1lon
Copy link
Member

eps1lon commented Sep 6, 2020

I don't think we can implement meaningful features since the most important ones rely on the build setup. A meta framework is far better suited to host such a component. There's a RFC in nextjs which has far better research and highlights why this component should leverage the build setup: vercel/next.js#16832

@eps1lon
Copy link
Member

eps1lon commented Nov 13, 2020

The only feature that we can meaningful implement is responsiveness considering we have breakpoints in the theme. All the other features are better suited for meta-frameworks.

@oliviertassinari
Copy link
Member Author

other features are better suited for meta-frameworks

Which comes with a drawback: portability. What works in Next.js doesn't in Gatsby or CRA.

@vicasas
Copy link
Member

vicasas commented Jan 11, 2021

Would this bring support for <picture /> and <figure /> ?

@AGDholo
Copy link
Contributor

AGDholo commented Feb 4, 2021

Can I use the element inside the component to have the image as a background image?

Such as:

<Image src="xxx">
  <div />
</Image>

@vicasas
Copy link
Member

vicasas commented Mar 6, 2021

@oliviertassinari @eps1lon In my team we had the need to have an Image component to improve the user experience when browsing our e-commerce site. We saw many solutions and dared to create a Material UI based component from scratch.

Here is a Codesandbox with a POC that I built, maybe it can be used in something for the construction of this much desired component and start working on it (:

Features:

  • Slow image loading
  • Fallback loading
  • Fallback error

https://codesandbox.io/s/material-ui-image-8icbi?file=/src/App.js

@jonah0000

This comment was marked as resolved.

@siriwatknp
Copy link
Member

I think the solution to this issue is to bring AspectRatio component to Material UI.

As a UI library, it makes more sense to provide a component that helps control the layout and leave img to the consumers. The AspectRatio works well with frameworks like NextJS, Imgix (we could add more to the list). cc @DiegoAndai

@DiegoAndai
Copy link
Member

@siriwatknp, that makes sense. Which of the description's motivation features could we cover with the AspectRatio component, and which would need another approach?

@siriwatknp
Copy link
Member

siriwatknp commented Nov 21, 2023

@DiegoAndai I should not have used the word solution in my previous comment. The AspectRatio component is not the solution to things like fallback, loading, and placeholder. In fact, those issues are already fixed by meta-framework as @eps1lon mentioned in #22470 (comment).

My point is that we should leave the Image component to meta frameworks and provide a layout component (AspectRatio) as a companion. So, I think this issue should be closed, and another one should be created for adding AspectRatio to Material UI.

@DiegoAndai
Copy link
Member

@siriwatknp Agree. Would you open a new issue for adding the AspectRatio and close this one when that's done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request waiting for 馃憤 Waiting for upvotes
Projects
None yet
Development

No branches or pull requests

8 participants