rpearce / react-medium-image-zoom

🔎 🏞 The original medium.com-inspired image zooming library for React (since 2016)
https://rpearce.github.io/react-medium-image-zoom/
BSD 3-Clause "New" or "Revised" License
1.88k stars 100 forks source link

styles for zoomImage prop don't work #122

Closed 13806 closed 4 years ago

13806 commented 6 years ago

Expected behavior

I expected style key to work with the zoomImage prop.

e.g.

zoomImage={{
    src: `${item.imageS}`,
    style: {width: `20rem`, height: `5rem`}
}}

...makes the zoomed image 20rem wide, 5rem tall. Changing styles works perfectly for the image prop.

Actual behavior

Adding style key the zoomImage prop object has no effect.

e.g.


zoomImage={{
    src: `${item.imageS}`,
    style: {width: `20rem`, height: `5rem`}
}}
rpearce commented 6 years ago

Hey I’m getting married today so can’t look at this for a little bit... I’ll get around to it in a week or so. Feel free to make a pull request if you so please!

On Feb 18, 2018, at 06:35, plkuzmich notifications@github.com wrote:

Expected behavior

I expected style key to work with the zoomImage prop.

e.g.

zoomImage={{ src: ${item.imageS}, style: {width: 20rem, height: 5rem} }} ...makes the zoomed image 20rem wide, 5rem tall. Changing styles works perfectly for the image prop.

Actual behavior

Adding style key the zoomImage prop object has no effect.

e.g.

zoomImage={{ src: ${item.imageS}, style: {width: 20rem, height: 5rem} }}

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

rpearce commented 6 years ago

@13806 Thank you for reporting this. Could you please provide the version of react-medium-image-zoom you are using and provide a full example of this not working? A link to a GitHub project or equivalent would do just fine.

rpearce commented 6 years ago

@13806 checking back in on this, and I'll repeat my question from before:

Could you please provide the version of react-medium-image-zoom you are using and provide a full example of this not working? A link to a GitHub project or equivalent would do just fine.

rpearce commented 6 years ago

Closing until I can get some more information.

deadcoder0904 commented 5 years ago

Hey, @rpearce I am facing the same issue.

Fortunately, I have a repro → https://github.com/deadcoder0904/unsplash-styled-components

The demo is at → https://unsplash-styled-components.netlify.com/

The related code is at → src/components/Pic.js

As you can see in Pic.js, I have specified styles in zoomImage

<ImageZoom
        image={{
          ...imageProps,
          style: {
            width: '37rem',
            height: '48rem',
          },
        }}
        zoomImage={{
          ...imageProps,
          style: {
            width: '102.4rem',
            height: '72rem',
          },
        }}
      />

But it doesn't work.

Basically, what I want to do is I want to have tall images when in a grid layout but when clicked I want the image to be wide.

So 370x480 when all the images are shown in Grid (tall) & 1024x720 when an image is clicked (wide).

I hope u understood what I said :)

rpearce commented 5 years ago

@deadcoder0904 Do you think there crossover between this issue and the PR that @xerona has opened? https://github.com/rpearce/react-medium-image-zoom/pull/156/files

deadcoder0904 commented 5 years ago

I thought so too. I installed that PR with ⸺

yarn add rpearce/react-medium-image-zoom#156/head 

But it didn't work out either. Is it because something is wrong with my code?

I pushed the new commit & updated the repo with the new PR → https://github.com/deadcoder0904/unsplash-styled-components

deadcoder0904 commented 5 years ago

I also tried the code mentioned here

<ImageZoom
  image={{src: path}}
  defaultStyles={{
    zoomContainer: {overflow: 'scroll'},
    overlay: { position: 'fixed', width: '100%', height: '100%' },
    image: { width: '100%' },
    zoomImage: { width: '100%', height: 'initial', transform: 'none', top: '0', left: '0', padding: '50px' }
  }}
/>

But it didn't work either.

rpearce commented 5 years ago

Okay, this and that PR are going on my to-do list. I don't have time to look at this during the work day, unfortunately. I'll check back in the next couple of days

rpearce commented 5 years ago

@deadcoder0904 THANK YOU for the repro

deadcoder0904 commented 5 years ago

This is the commit where I've used the PR version.

I got an error now Module not found: Can't resolve 'koa' in 'node_modules/react-medium-image-zoom' so I'll be using the npm version I think. But that commit should provide the repro needed :)

xerona commented 5 years ago

@deadcoder0904 If I understand your scenario correctly, here's why the style prop looks like it isn't working. I say looks like because I believe it will work right for any style except top, left, width, height, translate3d. You make the ImageZoom component and provide width and height as items of the object you're using in the argument style of both image and imageZoom. When <Zoom> is created, it sets the style on <img> with the results of _getZoomImageStyle. The results of _getZoomImageStyle are

Object.assign(
    {},
    defaults.styles.zoomImage,
    this.props.defaultStyles.zoomImage,
    this.props.style,
    style,
    zoomStyle
)

At this point, your {style: height, width} is stored this.props.style. style has also been computed. It contains the keys top, left, width, height. Because the returned value of _getZoomImageStyle is using Object.assign, your values for width and height are overwritten by subsequent assignments of those values.

tl;dr

const foo = Object.assign(
    {},
    defaults.styles.zoomImage,
    this.props.defaultStyles.zoomImage,
    {  // this.props.style,
        width: '102.4rem',
        height: '72rem',
    },
    {  // style
        top: image.getBoundingClientRect().top
        left: image.getBoundingClientRect().left
        width: image.width
        height: image.height
    },
    zoomStyle  // the translate3d with scale happens here
)
foo.width = image.width
foo.height = image.height
deadcoder0904 commented 5 years ago

So @xerona do I have to change the node_modules file with this or your PR will do it? A bit confused as to what I should change.

xerona commented 5 years ago

The code I included in the previous comment was just for understanding. If you want to test the code on my branch try this. yarn add https://github.com/xerona/react-medium-image-zoom.git#patch-1

If that doesn't work and you really need it now, you can implement a hack as I did.

import React, { Fragment } from "react";

import ImageZoom from "react-medium-image-zoom";
import Zoom from "react-medium-image-zoom/lib/Zoom";
import EventsWrapper from "react-medium-image-zoom/lib/EventsWrapper";

export class ZoomHack extends Zoom {
  _getZoomImageStyle() {
    return Object.assign(
      super._getZoomImageStyle(),
      this.props.style,
      this.props.defaultStyles.zoomImage
    );
  }
}

const isControlled = isZoomed => isZoomed !== null && isZoomed !== undefined;
const focusableTabIndex = 0;
const unfocusableTabIndex = -1;

export default class ImageZoomHack extends ImageZoom {
  render() {
    const {
      props: {
        defaultStyles,
        image,
        isZoomed: propsIsZoomed,
        shouldRespectMaxDimension,
        zoomImage,
        zoomMargin
      },
      state: { isDisabled, isZoomed: stateIsZoomed, src, isClosing }
    } = this;

    const attrs = Object.assign(
      {},
      !isDisabled && { tabIndex: focusableTabIndex },
      image,
      { src, style: this._getImageStyle() },
      !isDisabled && {
        onMouseDown: this._preventFocus,
        onClick: this._handleZoom,
        onKeyDown: this._handleKeyDown
      }
    );
    const isZoomed = isControlled(propsIsZoomed)
      ? propsIsZoomed
      : stateIsZoomed;

    return (
      <Fragment>
        <img
          {...attrs}
          key="image"
          ref={x => {
            this.image = x;
          }}
          onLoad={this._handleLoad}
          onError={this._handleLoadError}
        />
        {this.image && (isZoomed || isClosing) ? (
          <EventsWrapper
            key="portal"
            ref={node => {
              this.portalInstance = node;
            }}
            controlledEventFn={this._getControlledEventFn()}
            allowAccessibilityClose={this._allowTabNavigation()}
          >
            <ZoomHack
              defaultStyles={defaultStyles}
              image={this.image}
              shouldRespectMaxDimension={shouldRespectMaxDimension}
              zoomImage={zoomImage}
              zoomMargin={Number(zoomMargin)}
              onUnzoom={this._handleUnzoom}
            />
          </EventsWrapper>
        ) : null}
      </Fragment>
    );
  }
}

Then just create the new thing in the same way.

    <ImageZoomHack
        image={{
          ...imageProps,
          style: {
            width: '37rem',
            height: '48rem',
          },
        }}
        zoomImage={{
          ...imageProps,
          style: {
            width: '102.4rem',
            height: '72rem',
          },
        }}
      />
deadcoder0904 commented 5 years ago

Naah I can't use a hack as it's needed for a tutorial. I can't explain readers this 😂but thanks for the info :)

rpearce commented 5 years ago

At some point, this library is going to get a lot easier to work with. That doesn't help you right now, though. Again, this is on my to-do list for this week.

rpearce commented 5 years ago

I am sorry that this has fallen by the wayside, as I've been focused on https://github.com/rpearce/react-medium-image-zoom/issues/143 and https://github.com/rpearce/react-medium-image-zoom/pull/159.

Does https://github.com/rpearce/react-medium-image-zoom/pull/156 resolve the issue here?

rpearce commented 4 years ago

This should not be an issue in the latest react-medium-image-zoom@alpha v4 release channel, for we don't exactly have a zoomImage prop to work with in the new API (though I'll try to figure out how/if I want to achieve something similar in https://github.com/rpearce/react-medium-image-zoom/issues/166).

Closing this

rpearce commented 4 years ago

@13806 @deadcoder0904 @xerona I've added you all to the "all contributors" section as a thank you for your :eyes: and 💭 here.

image