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

onLoadingComplete() should pass naturalWidth and naturalHeight #27213

Closed
nickadamson opened this issue Jul 15, 2021 · 10 comments · Fixed by #27695
Closed

onLoadingComplete() should pass naturalWidth and naturalHeight #27213

nickadamson opened this issue Jul 15, 2021 · 10 comments · Fixed by #27695
Assignees
Milestone

Comments

@nickadamson
Copy link

What version of Next.js are you using?

11.0.2-canary.16

What version of Node.js are you using?

14.16.0

What browser are you using?

Brave (Chromium)

What operating system are you using?

Windows

How are you deploying your application?

next dev

Describe the Bug

onLoadingComplete() event handler does not pass any props.

Expected Behavior

The expected behavior for onLoadingComplete (unless I'm missing the point of this prop) is the same as onLoad for regular old HTML and onLoadedMetadata/onCanPlay for videos -- a synthetic react event with .target prop.

To Reproduce

I have a function/event handler that calculates the aspect ratio of an image as soon as it is loaded:

const calcAspectRatio = (loadedMedia) => {

      console.log("loadedMedia", loadedMedia)

      let ratio;

      let width;

      let height; // ... you get the idea. bug can be replicated with a simple console.log though. 

Here's how I've implemented next/image:

<Image layout="fill" src={media} onLoadingComplete={calcAspectRatio} />

and videos:

<video muted autoPlay={autoPlay} loop onLoadedMetadata={calcAspectRatio}>
    <source src={media} />
</video>

Currently the calcAspectRatio returns undefined for next/image - afaiu, this would mean it doesn't pass any reference to itself when loading is complete

And returns this (expected) for video:

SyntheticBaseEvent {_reactName: "onLoadedMetadata", _targetInst: null, type: "loadedmetadata", nativeEvent: Event, target: video.nfte__media-content, …}

buggitybugbug

@nickadamson nickadamson added the bug Issue was opened via the bug report template. label Jul 15, 2021
@nickadamson
Copy link
Author

see #27152 also

@leerob
Copy link
Member

leerob commented Jul 23, 2021

^ Locked #27152 and moved the discussion here.

@botv
Copy link
Contributor

botv commented Jul 23, 2021

This appears to have been fixed in (or by) 11.0.2-canary.20

@styfle
Copy link
Member

styfle commented Jul 27, 2021

You'll need to updated to the latest canary to get onLoadingComplete() prop #18398 (comment)

We are not providing the ref/element at this time because it would be too easy to break the component if you had raw access to the underlying image, for example changing inline styles.

In your example where you use calcAspectRatio(), what does the aspect ratio get used for? The component should handle its own aspect ratio.

@nickadamson
Copy link
Author

nickadamson commented Jul 29, 2021

@styfle calcAspectRatio() takes the width and height of original image, calculates the aspect ratio (as well as portrait/landscape orientation) and sets state to ${orientation} ${aspect-ratio}. The state is used in a className, which tells the div how many grid columns/rows to span.

Basically a messy, dense gallery grid from a stream of photos outside of my control. Think tetris, but photos of various aspect ratios.

@styfle
Copy link
Member

styfle commented Jul 29, 2021

You can find the width & height from the parent element since layout=fill will fill to the dimensions of the parent.

For example, this parent div is 300x500 so the image will also be 300x500.

<div style={{ position: 'relative', width: '300px', height: '500px' }}>
<Image alt="Mountains" src={mountains} layout="fill" objectFit="cover" />
</div>

Demo: https://image-component.nextjs.gallery/layout-fill

@nickadamson
Copy link
Author

nickadamson commented Jul 29, 2021

@styfle yeah, I get that, and honestly I might be expecting too much from next/image. But basically I'm setting the width and height of parent element after image is loaded.

> Have grid with auto columns at 10px wide
> Image URI's are fetched from API
> For each image URI -> create hidden div with child <Image/img>
> Once image is loaded -> determine its aspect ratio
> change hidden div to div with aspect ratio of image
> for example, css classes for 'portrait 2x3' : grid-column-span 20, grid-row-span 30

This is the look I'm trying to achieve ![image](https://user-images.githubusercontent.com/80008674/127542339-d445b6db-a6f6-4431-a278-92d8b84ab8a2.png)

I could show everything as squares, but the site is for artists and I'd like to respect their creative decisions, like composition.
Honestly if there was a way to get image's metadata over HTTP that would be amazing, but I haven't found a simpler way to do this and I've been tackling this problem for at least a month now.
I'd really like to use next/image as some of the content that gets pulled is ridiculously large 8000px width files. GPU usage can be 50% on my machine depending on what's been published recently. And that's only with 12 items loaded. If next/image allowed the ref the function would be able to run.

Also, if next/image just had a fileAspectRatio prop that could work I think, but I don't know what I'm talking about in regards to image optimization.

E2A: At this point, I've misunderstood the purpose of the onLoadingComplete, feel free to close the issue if discussion isn't helpful.

@styfle
Copy link
Member

styfle commented Jul 29, 2021

If onLoadingComplete() passed back naturalWidth and naturalHeight, would that solve the problem?

@nickadamson
Copy link
Author

@styfle 100% yes, it would. I was unaware of those properties.

@styfle styfle changed the title next/image onLoadingComplete() does not behave like usual React onLoad() onLoadingComplete() should pass naturalWidth and naturalHeight Jul 31, 2021
@styfle styfle added kind: story and removed bug Issue was opened via the bug report template. labels Jul 31, 2021
@styfle styfle self-assigned this Jul 31, 2021
@styfle styfle added this to the Iteration 23 milestone Jul 31, 2021
@kodiakhq kodiakhq bot closed this as completed in #27695 Aug 2, 2021
flybayer pushed a commit to blitz-js/next.js that referenced this issue Aug 19, 2021
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants