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

Feature: Add scale prop #434

Closed
wants to merge 8 commits into from
Closed

Feature: Add scale prop #434

wants to merge 8 commits into from

Conversation

Tenpi
Copy link
Contributor

@Tenpi Tenpi commented Aug 14, 2021

Hello. I am using this together with react-zoom-pan-pinch in order to enable zooming + cropping. However, there is a problem where if the image is inside a parent div that is scaled, the cropping bounds will not follow the mouse cursor at all. I found an old PR that solves this exact same issue (#170), but I'm really not sure why it wasn't merged, maybe they just didn't do a good job explaining (since the name delta is confusing).

Current behavior when cropping a scaled image (doesn't follow the mouse at all):

cropNoScale

With the scale prop set:

cropScale

I apologize for the laggy recording, but you can tell that it follows the mouse cursor a lot better.

Usage in code:

<TransformWrapper onZoomStop={(ref) => setZoomScale(ref.state.scale)}>
    <TransformComponent>
        <div className="photo-container">
            <ReactCrop className="photo" src={image} scale={zoomScale} crop={cropState} 
              onChange={(crop, percentCrop) => setCropState(percentCrop)} 
              disabled={!cropEnabled} keepSelection={true}/>
        </div>
     </TransformComponent>
 </TransformWrapper>

@Tenpi Tenpi changed the title Feature: Add scale prop Feature: Add scale and rotate props Aug 15, 2021
@Tenpi
Copy link
Contributor Author

Tenpi commented Aug 16, 2021

Edit: I have also added in a rotate prop in order to correct the bounds if a parent element is rotated.

Example with rotate prop set:

rotate

Example code usage:

<div className="rotate-container" style={{transform: `rotate(${rotateDegrees}deg)`}}>
    <ReactCrop className="photo" src={image} scale={zoomScale} rotate={rotateDegrees} crop={cropState} 
    onChange={(crop, percentCrop) => setCropState(percentCrop)} 
    disabled={!cropEnabled} keepSelection={true}/>
</div>

@sekoyo
Copy link
Owner

sekoyo commented Aug 17, 2021

This looks cool thanks, I'll check it out and merge over the next few days 👍

@sekoyo
Copy link
Owner

sekoyo commented Aug 19, 2021

@Tenpi what do you think about getting these props to work visually without adding more wrappers? Less work and confusion for users of the props, but less control and could break functionality of something like react-zoom-pan-pinch if it's already applying rotation and scaling itself?

const mediaStyle = {
  transform: `scale(${scale}) rotate(${rotate}deg)`,
};

return (
  <div
    ref={this.bindComponentRef}
    className={componentClasses}
    style={style}
    onPointerDown={this.onComponentPointerDown}
    tabIndex={0}
    onKeyDown={this.onComponentKeyDown}
    onKeyUp={this.onComponentKeyUp}
  >
    <div ref={this.bindMediaWrapperRef} style={mediaStyle}>
      {renderComponent || (
        <img
          ref={this.bindImageRef}
          crossOrigin={crossorigin}
          className="ReactCrop__image"
          style={imageStyle}
          src={src}
          onLoad={this.onImageLoad}
          onError={onImageError}
          alt={imageAlt}
        />
      )}
    </div>
    {children}
    {cropSelection}
  </div>
);

Result:

Screen.Recording.2021-08-19.at.14.07.26.mov

@sekoyo
Copy link
Owner

sekoyo commented Aug 19, 2021

Actually if I moved the scale and rotate outside the crop, it scales and rotates the whole thing. Maybe I am misunderstanding what part of this you want to scale and rotate?

Looking at getRotatedCursor maybe the intention is to rotate and scale the crop selection? Would it not be simpler to do it on the image as shown in the video above?

Screenshot 2021-08-19 at 14 24 56

@Tenpi
Copy link
Contributor Author

Tenpi commented Aug 19, 2021

Actually if I moved the scale and rotate outside the crop, it scales and rotates the whole thing. Maybe I am misunderstanding what part of this you want to scale and rotate?

Looking at getRotatedCursor maybe the intention is to rotate and scale the crop selection? Would it not be simpler to do it on the image as shown in the video above?

Yes, by scaling and rotating I do mean scaling and rotating the whole crop selection itself. In the first example that you showed you are scaling and rotating the image, but the crop selection maintains the same size. I am scaling/rotating the "view" or "canvas" that the image is on.

Maybe for the greatest flexibility, you could offer both options? I believe that in Photoshop, the crop selection scales with the canvas but rotation is applied to the image itself. If you offer both options then the user can just customize the behavior that they want.

@sekoyo
Copy link
Owner

sekoyo commented Aug 19, 2021

Ok I see, I'm thinking about only changing the image as it's such a simple change 🤔 You can see this is the standard approach in other cropping tools such as https://pqina.nl/pintura/ and https://www.npmjs.com/package/react-easy-crop

The thing I'm confused about with the approach from the screenshot above is that the entire thing (image and crop) is changed, so actually what's under the crop is still exactly the same since they are both rotating/scaling. I would imagine that you would rotate/scale either just the image, or just the crop? But I don't currently expose the ability to add styles to the crop (other than with css, not inline styles).

@Tenpi
Copy link
Contributor Author

Tenpi commented Aug 19, 2021

The thing I'm confused about with the approach from the screenshot above is that the entire thing (image and crop) is changed, so actually what's under the crop is still exactly the same since they are both rotating/scaling.

Oh yeah, you are right. My bad. Then it's probably better to go with your idea of applying the style directly onto the image.

However, are you still ok with keeping the scale prop? I'd still prefer to scale the whole canvas over just the image. (I will update this PR with the rotate prop removed).

@Tenpi Tenpi changed the title Feature: Add scale and rotate props Feature: Add scale prop Aug 19, 2021
@sekoyo
Copy link
Owner

sekoyo commented Aug 19, 2021

Are you able to attach a clip here showing the behaviour that you want as I'm not quite sure?

I have some questions like:

  • If the image AND crop are both enlarged, what is the intention behind that since you will end up with the same crop in the end- is it for the hard of sight, like zooming in the browser?
  • If they both enlarge does the container stay the same dimensions? Wouldn't this result in the crop potentially disappearing beyond the limits of the container in that case? And if the container does enlarge too, then wouldn't that break the layout of the page it's in?

@Tenpi
Copy link
Contributor Author

Tenpi commented Aug 19, 2021

  • If the image AND crop are both enlarged, what is the intention behind that since you will end up with the same crop in the end- is it for the hard of sight, like zooming in the browser?

It's just to make it easier to make more precise crops by being able to zoom in.

  • If they both enlarge does the container stay the same dimensions? Wouldn't this result in the crop potentially disappearing beyond the limits of the container in that case? And if the container does enlarge too, then wouldn't that break the layout of the page it's in?

They both scale. Can't say whether it breaks the page layout or not, since everything in my app (except for the image) is absolutely positioned.

The best explaining that I can do is to use the crop tool in Photoshop. When you zoom in/out, it zooms the entire canvas and not the image - that's pretty much what I want to mimic.

Alternatively, you can also try my app here: https://github.com/Tenpi/Photo-Viewer/releases
The zooming in/out works pretty much how I want it to. I have to change the rotation to only affect the image, because as you pointed out before it doesn't actually rotate.

@sekoyo
Copy link
Owner

sekoyo commented Aug 19, 2021

Cool thanks I think I get what you mean 👍 I might add two props to satisfy both like scale and zoom

@sekoyo
Copy link
Owner

sekoyo commented Aug 19, 2021

P.S I'm upgrading the project to Typescript at the same time so it might take me a few days

@sekoyo
Copy link
Owner

sekoyo commented Aug 20, 2021

This has been added as a zoom prop (there is also scale and rotate for behaviours only on the image)

@Tenpi
Copy link
Contributor Author

Tenpi commented Aug 20, 2021

This has been added as a zoom prop (there is also scale and rotate for behaviours only on the image)

Awesome! Thank you very much for the addition 👍

@Tenpi Tenpi closed this Aug 20, 2021
@Tenpi
Copy link
Contributor Author

Tenpi commented Aug 20, 2021

Sorry to bother you, but in the latest release I am getting the error:
TypeError: Cannot destructure property 'clientWidth' of 'this.mediaWrapperRef.current' as it is null.

In React, refs are only available after the component renders, so maybe set the width/height to 0 if the component hasn't rendered yet?

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

Successfully merging this pull request may close these issues.

None yet

2 participants