henrygd / bigger-picture

JavaScript lightbox gallery for images, video, audio, iframes, html. Zoomable, responsive, accessible, lightweight.
https://biggerpicture.henrygd.me
MIT License
231 stars 17 forks source link

Cannot read properties of undefined (reading 'getBoundingClientRect') #4

Closed kctdfh closed 2 years ago

kctdfh commented 2 years ago

Hey - really like the simplicity of the library! Great job!

I thought it'd be an easy drop-in replacement for an existing project but I can't seem to load up any lightboxes without getting a specific error in the console and not being able to change slides or close out of the lightbox.

Here's a reproduction on Codepen. As you can see, the "videos" lightbox, which has html content, loads but hangs. But the "photos" lightbox doesn't load past the overlay and captions.

https://codepen.io/kctdfh/pen/yLpdWBd

On the Codepen console there are no errors, it seems. But with the same setup running locally, I get this in my console:

bigger-picture.min.js:1 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'getBoundingClientRect') at H (bigger-picture.min.js:1:20786) at Bt.at.x (bigger-picture.min.js:1:21440) at tt (bigger-picture.min.js:1:3331) at bigger-picture.min.js:1:18126 at J (bigger-picture.min.js:1:2606) H @ bigger-picture.min.js:1 Bt.at.x @ bigger-picture.min.js:1 tt @ bigger-picture.min.js:1 (anonymous) @ bigger-picture.min.js:1 J @ bigger-picture.min.js:1 Promise.then (async) (anonymous) @ bigger-picture.min.js:1 (anonymous) @ bigger-picture.min.js:1 Bt.$ @ bigger-picture.min.js:1 showVideoLightbox @ scripts.js:1

It's probably an obvious oversight on my end but I've been trying a bunch of things and can't make it work. Since v1 was just released, I thought maybe it also could be something you'd want to know about.

Cheers

henrygd commented 2 years ago

Thanks, this is definitely something I want to know about. As you said, v1 was just released and there hasn't been a lot of real world testing yet, so it could be an issue with the library.

I'll have some time to check this out in a couple of hours and will follow up then.

henrygd commented 2 years ago

Okay, so what's happening here is that the library is trying to run the default zoom entrance animation but doesn't have an element to start from. When you are passing data via JS object, you need to provide an existing element on the page for that.

My fault that this isn't made clear in the readme. I did mention it here, but it's not in the example and there's no warning that it's required. I'll make that more clear, and I can probably add a fallback or at least handle the error better in the code.

I took your example and slightly modified it to make this codesandbox:

https://codesandbox.io/s/heuristic-benji-fjl8ep?file=/script.js

The important bit is the element property added to the itemsVideos array. The other option is to use the fadeup intro, like I added to the photos. No more errors after that.

You might want to change the video to use the sources option with defined height / width, so that scales properly. If the problem is that you need different attributes I can show you how to do that.

kctdfh commented 2 years ago

Very nice! Looks great now! Yeah I think the bit about passing an object needs more clarification! About the video, yeah I'm looking to autoplay some videos and have webm animations instead of GIFs in other places. What I have now is based on what I needed for my previous lightbox solution.

Would you suggest I set those with a function passed to onOpen?

henrygd commented 2 years ago

Exactly, you can use onOpen like below. If you have videos in a gallery with multiple items you can do the same with onUpdate. And if you're mixing and matching types, you can use activeItem to make sure it's a video. Also, if your video is 1920x1080, you don't need to supply the height / width because that's the default.

bp.open({
  onOpen(container, activeItem) {
    let video = container.querySelector('video')
    video.setAttribute('name', 'value')
  },
})

Anyway, hope it works out for you and let me know if you have any other questions. You can close the issue now if you want, or I'll close it after I push an update to handle this better.

kctdfh commented 2 years ago

Thanks a lot for the help! The library is really straight forward!

I have one question about the size actually. I'm noticing that the height and width I supply to an item seems to be forcing the aspect ratio. Is that correct?

I have a lot of irregular aspect ratios and no trivial way to know the measurements of an image/video before loading it. Is there a way to force a "contain" type behavior so that the image fills the height on wide screens and the width on tall screens?

I could do something like that with onUpdate maybe or even CSS but I first wanted to see if there was a built in way. I checked your code on the first demo and I seem to be doing it the way you have your sizes set but my images end up stretched.

henrygd commented 2 years ago

Unfortunately for zooming and the default entrance animation you really need to know width / height beforehand. Ideally you'd be pulling from a source (database / api) that would give you access to that info. If you're using webpack / rollup with local files, there may be a plugin that can process the images to give the dimensions. Or if you're loading thumbnails on a page in the correct aspect ratio you could grab those dimensions with javascript and multiply by 10 or something.

If the demo you're talking about is the one below, I have the dimensions set as data-attributes in the html.

https://codesandbox.io/s/bigger-picture-basic-gallery-o4kb82

henrygd commented 2 years ago

Added focused element as a fallback and made the docs more clear for 1.0.1.