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

Smarter detection for image naturalWidth and naturalHeight #213

Closed
rpearce opened this issue Jan 19, 2020 · 17 comments
Closed

Smarter detection for image naturalWidth and naturalHeight #213

rpearce opened this issue Jan 19, 2020 · 17 comments
Assignees
Labels

Comments

@rpearce
Copy link
Owner

rpearce commented Jan 19, 2020

Smarter detection for image naturalWidth and naturalHeight so we don't try to zoom anything when it's already at its maximum dimensions. This would also re-enable the ability to not zoom beyond a source image's natural dimensions once zoomed.

In v3, there was a flag, shouldRespectMaxDimension, but I think that should be the default for image-based children.

Also, just like v3, if it's already the full-width of the page (or container?), don't zoom it.

@rpearce
Copy link
Owner Author

rpearce commented May 25, 2020

FYI I'm almost done completely rewriting this library into:

  1. a vanilla JS package that is ~3kb (minified & gzipped)
  2. a react hook that uses 👆 and is maybe 100-150 bytes

This and all the other issues should be closed out by it. I'm almost there (working out some API stuff).

Thanks for your patience

@rpearce rpearce mentioned this issue Jun 18, 2020
3 tasks
@tommoor
Copy link
Contributor

tommoor commented Mar 30, 2021

I'm stuck back on v3 until this lands, is it worth PRing something for v4 or does the ongoing rewrite make it a waste of time? :)

@rpearce
Copy link
Owner Author

rpearce commented Mar 30, 2021

I wouldn't bother with trying anything like that with v4, for the fundamental concept of v4, "zoom anything", was flawed.

v5 is going to pull that back to images.

You can read more about the plight here: #274

@tommoor
Copy link
Contributor

tommoor commented Mar 30, 2021

Ahhh, thanks for the link, I hadn't looked in discussions. One thing that I did like about the idea behind v4 was that it would potentially allow us to put information such as a caption underneath the image, or potentially < > buttons to navigate a set – just something to think about as you design the v5 API.

Many thanks

@rpearce
Copy link
Owner Author

rpearce commented Mar 30, 2021

@tommoor Nothing is set for v5 apart from some core things that need to happen (fixing pixelation on iOS and accessibility, for example).

I'm not yet saying I will support what you're asking about, but I would love to know what your use case is so that I can identify how that might fit in. For example, I love what the New York Times has done.

@tommoor
Copy link
Contributor

tommoor commented Mar 30, 2021

Use case is a rich text editor with user-editable captions. The NYT design is great you're right, would certainly be nice to have something with that much panaché.

Edit: FWIW NYT's is React-driven, you can see the types of props they're providing:

image

@madeleineostoja
Copy link

madeleineostoja commented Apr 2, 2021

I'll also echo @tommoor, not being able to include non-image things in the zoom would be a dealbreaker for me, and it's the whole reason I used this library over others. My particular use-case is showing a title under the image when it's zoomed, like a more traditional lightbox might. I'm also using the controlled mode to only show the title when it's zoomed. The problem of course is that the zoom also blows the title up with the scale transform, not sure how to solve that one...

Could we perhaps get access to the zoom factor when in controlled mode, so we can account for it in anything that shouldn't be scaled up?

@rpearce
Copy link
Owner Author

rpearce commented Apr 3, 2021

@madeleineostoja I'll make sure to bake a caption option in.

Could you further explain what you mean by this?

Could we perhaps get access to the zoom factor when in controlled mode, so we can account for it in anything that shouldn't be scaled up?

@rpearce
Copy link
Owner Author

rpearce commented Nov 10, 2021

I've got this covered in the next major version I'm working on. Update here: #274 (comment)

@rpearce
Copy link
Owner Author

rpearce commented Jun 14, 2022

Here's the latest update on the work that will include this! #274 (reply in thread)

@rpearce
Copy link
Owner Author

rpearce commented Jul 10, 2022

Hi! Here's a screencast of the max dimension being respected in the v5-dev branch:

rmiz-zoom-clamp.mp4

Once v5.0 is released (hopefully not too much longer), I'll see about adding in proper captions and maybe do so by looking for a <figcaption> or [data-rmiz-caption] (plan for a v5.1 release)

@tommoor
Copy link
Contributor

tommoor commented Jul 10, 2022

Your hard work is appreciated 🙏

@rpearce
Copy link
Owner Author

rpearce commented Jul 25, 2022

If anyone finds some spare time and wants to give the latest build a whirl to try this out:

npm i --save react-medium-image-zoom@5.0.0-beta2.8

And, if you aren't already,

import Zoom from 'react-medium-image-zoom'
import 'react-medium-image-zoom/dist/styles.css'

// ... your other code

<Zoom>
  <MyImage />
</Zoom>

The updated v5-dev README is here, and there are some breaking changes to the extra options passed to be aware of, if you're using the options.

Thanks for your time!

@rpearce
Copy link
Owner Author

rpearce commented Aug 3, 2022

Should be closed now with the release of 5.0.0

@rpearce rpearce closed this as completed Aug 3, 2022
@rpearce
Copy link
Owner Author

rpearce commented Aug 6, 2022

@tommoor @madeleineostoja

Hi! I'm working on adding a feature that includes captioning (adding whatever you want to the zoom state). Do you have any thoughts on this approach? #332 (comment)

@tommoor
Copy link
Contributor

tommoor commented Aug 7, 2022

Example looks good and would certainly work for our usecase. I would consider just passing an unZoom/onClose button as prop to custom component rather than a rendered button.

@rpearce
Copy link
Owner Author

rpearce commented Aug 7, 2022

@tommoor Good call on that line of thinking! I'll ponder it and see where that goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants