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

Add support for images with different sizes #127

Open
jahirfiquitiva opened this issue Jan 29, 2024 · 10 comments
Open

Add support for images with different sizes #127

jahirfiquitiva opened this issue Jan 29, 2024 · 10 comments
Labels
enhancement New feature or request has workaround Issue has a viable workaround

Comments

@jahirfiquitiva
Copy link

jahirfiquitiva commented Jan 29, 2024

Hey, I'm trying to use this lib, which looks pretty good, although I'm missing a small feature

I was using react-compare-image before, and it has this aspectRatio prop feature, allowing for the slider to look fine when images aspect ratios differ, and therefore being able to view both images in full size.

Shot 2024-01-29 at 12 09 01@2x

Here's their examples:
https://react-compare-image-git-master-junkboy0315.vercel.app/?path=/story/images-with-different-aspect-ratios--default-behavior

I created a sandbox showing the current behavior

https://codesandbox.io/p/devbox/infallible-shape-t5p8tl?file=/src/App.tsx:16,10

Would it be possible for you to add such feature? Or is it already possible? And if it is, how to set it up?

Thanks in advance!

@nerdyman
Copy link
Owner

Hi @jahirfiquitiva, that's an interesting use case!

There's no built-in prop to do this but you could achieve what you need with a bit of extra logic.

The most foolproof way would probably be to attach a resize observer to the slider component and refs to the images to get their natural widths and heights and then calculate their aspect ratios. You'd then opt for the higher or taller one. One thing to note is that it looks like react-compare-image uses fixed widths and heights on the slider where as this lib uses the intrinsic sizing of itemOne so you'd also want to set a fixed width and height on the slider component itself if you want to mimic what react-compare-image does.

I'll take a look at getting a demo set up, if it ends up being complex I think it would be worth adding a prop to this lib similarly to react-compare-image.

@jahirfiquitiva
Copy link
Author

Thanks for your reply @nerdyman .. A demo would be incredibly helpful. Thanks in advance!

@nerdyman
Copy link
Owner

@jahirfiquitiva I've got a basic demo working here https://codesandbox.io/p/sandbox/rcs-aspect-ratio-forked-fngydc?file=%2Fsrc%2FApp.tsx%3A12%2C1-13%2C1

It starts with the default behaviour (shown as undefined) then cycles through undefined|'taller'|'wider'. I ended up using the aspect-ratio CSS prop rather than setting fixed widths and heights on things which is a bit cleaner IMO.

This is still fairly basic since it won't automatically change the aspect ratio if you change the images in itemOne and itemTwo. To do that I'd either use a resize observer to fire handleAspectRatio() on resize or a useEffect to fire handleAspectRatio() on prop changes.

The logic is a bit involved so I'll have a think about how to best add this to the lib but this should hopefully get you going in the meantime!

@jahirfiquitiva
Copy link
Author

jahirfiquitiva commented Jan 30, 2024

@nerdyman it says Sandbox not found, maybe it isn't public? 😬

@nerdyman
Copy link
Owner

@jahirfiquitiva should be ok now 🙏

@jahirfiquitiva
Copy link
Author

@nerdyman wow, thank you so much for the help. It seems to work fine in the demo, I'll give it a try in my project later and let you know how it goes. Thanks again! 🎉

@nerdyman nerdyman added enhancement New feature or request has workaround Issue has a viable workaround labels Jan 30, 2024
@nerdyman
Copy link
Owner

@jahirfiquitiva Happy to help 😄 let me know how it goes!

@jahirfiquitiva
Copy link
Author

jahirfiquitiva commented Feb 3, 2024

hey @nerdyman , it took me a bit to get back to this, sorry ... Anyway, I think this doesn't seem to work quite fine. Probably because the images are bigger than the container? 🤔

This is the result with your demo code:

This is the expected result:

I've made a PR to my website with these changes, in case you could take a look there 🙏

jahirfiquitiva/jahir.dev#74

Thanks in advance!

@nerdyman
Copy link
Owner

nerdyman commented Feb 3, 2024

Hi @jahirfiquitiva, Thanks for proving the link to your use case 🙌

I think it's because the slider is still using the bounds of the first image. In the sandbox the slider is filling the width of the container so the images fit better.

I was thinking of adding a separate hook to the lib to handle this so I'll take a look at your repro and figure something out.

@jahirfiquitiva
Copy link
Author

@nerdyman I kinda fixed it by using the container width to calculate its aspect ratio and waiting for the images to load

jahirfiquitiva/jahir.dev@f628fab (#74)

Anyway, it causes a layout shift.

Shot.2024-02-03.at.11.19.16.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request has workaround Issue has a viable workaround
Projects
None yet
Development

No branches or pull requests

2 participants