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

Aspect ratio, crop data, original image data, and fix for hidden canvas #48

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

iblank
Copy link

@iblank iblank commented Jan 26, 2015

Quick note: I don't have Gulp installed, so I didn't create the compiled files that way. I manually pieced together the compiled, unminified version of the JS file though.

Here is a list of updates:

  • aspect ratio
    • passed in as a string (width + 'x' + height)
    • defaults to 1x1 (square)
    • adjusted the square and circle classes to work as rectangle and ellipses classes
  • passes crop data and original image data, as attributes
    • crop data = { width, height, x, y }
    • original data = { width, height }
    • crop data is 2-way binding, so that users can initiate the canvas to a specific crop
      • if initiated out of bounds, it adjusts values to fit
    • X and Y coordinates are based on left-top corner
  • fixes issue with canvas redrawing incorrectly after being hidden
    • when the canvas is hidden it's clientHeight is set to 0, which basically tells the code to redraw everything to minimum values
    • added an if statement to prevent redrawing when the clientHeight is 0
  • initializes the crop to the maximum possible from the middle of the image
    • this is useful in projects where using the cropping tool is optional to the user, and creating a cropped image (to fit a particular aspect ratio) is required on the back-end
  • updated the test HTML file to show the recent changes
  • updated the README.md to reflect some of the recent changes

@iblank
Copy link
Author

iblank commented Jan 26, 2015

I apologize for the long description and change list. I have been making these changes over time to a private repository, and finally got around to contributing them back here.

@jonmikelm
Copy link

Tested the new option for setting the aspect ratio of the result image and works like a charm.

Just one thing, initializing the crop to the maximun size is not very useful in my case. Im using ngImgCrop in a mobile app, and it's a bit tricky for the user to resize the crop when it's size is the maximun. The touch is not as precise as the mouse, and it's a bit difficult to hit the resize points when they are in the border of the container layer. I think it would be good to parametrize this option.

@iblank
Copy link
Author

iblank commented Feb 17, 2015

Sounds like a good suggestion to me. I haven't looked at it in a while, but I believe the original default was half the width of the canvas. This could be the standard default again, with an option to maximize the selection.

@iblank
Copy link
Author

iblank commented Feb 18, 2015

I decided to change the default crop selection to 75% of the closest side (based on aspect ratio). I think 50% worked well when the crop selection was always an even square or circle, but that it could be too small in cases where the aspect ratio of the crop and image differ too much. If you set the 'maximize-crop' attribute to true, it'll set the selection to 100%.

@FyerPower
Copy link

I love all of the changes you have made, i do wish @alexk111 gets the time to review and hopefully merge these. Thanks for your work!

@iblank
Copy link
Author

iblank commented Feb 25, 2015

No problem @gamegenius86. We needed all this functionality in our own project, but it's nice to hear it's helping others.

@iblank
Copy link
Author

iblank commented Feb 25, 2015

A side note on the latest commit... the img-crop-result directive approach is meant to replace the canvas.toDataURL() function, since it's not supported in IE9. It's a better solution when you actually do all the cropping work on the backend. I've found that it's considerably faster as well, since it's not generating a new image every time the crop is adjusted.

Sample usage:

<div img-crop-result image="image.imagePath" crop-data="image.cropData" width="60%"></div>

If submitting an already cropped image to your backend is required, then I would stick to the original result-image attribute to generate that image.

@Capellaro
Copy link

Seems to be really great work ! Is it possible to import it without waiting the pull request to be merged?

@generics
Copy link

great job thank you.

@iblank
Copy link
Author

iblank commented Mar 10, 2015

@Alexcapi you can import my forked version here: https://github.com/iblank/ngImgCrop

Update the crop-host.js file to fix an issue with images taken from devices with high MP cameras. Reduce canvas size to 1000px if required.
@hbbh
Copy link

hbbh commented Mar 23, 2015

@iblank Thanks for your great work on this. I created a pull request this morning, which uncomments out the section of your code handling the EXIF rotation and also fixes an issue that is currently present in the original repo where large images taken from devices are not rending in the edit window.

@daanl
Copy link

daanl commented Mar 30, 2015

@alexk111 can you accept this?

@STRML
Copy link

STRML commented May 4, 2015

👍

@daanl
Copy link

daanl commented May 18, 2015

@alexk111 can you accept this?

@tinquang
Copy link

tinquang commented Jul 9, 2015

Thanks for very helpful feature @iblank .
I found that crop-data is caculated base on scale image, not the original one. Because in my case, I upload the original image with the crop data, but because I display the image in responsive view so the upload crop-data can not apply to the original image.
Can you give me some hints to fix this case?
Thanks.

@iblank
Copy link
Author

iblank commented Jul 9, 2015

I'm sorry, I don't exactly follow what your issue is. Are you saying that the crop data you pass in is not scaling correctly on the outputted image? The crop box should be using the same scale factor as the original, so that the output matches what you see. The crop data you pass in needs to be based on the actual dimensions of the original. Maybe you can post a screen capture that illustrates what's happening?

@tinquang
Copy link

ok, let me explain, for example I have a portrait image with original size is 960x1280, but when I show it up for cropping, I set container style limit height to 400, so the display image will be 300x400, and when I make a crop, the crop-data return crop information of the display image, not the original one, that mean the maximum crop size (circle) will be 300x300, and because I will upload the image file(not the base64 data) with the crop-data, the result image will has wrong size and wrong position.

@iblank
Copy link
Author

iblank commented Jul 10, 2015

Ok, it sounds to me like the issue is likely caused by the use of max-height... ngImgCrop reads the width and height of the image, and the width and height of the canvas in order to scale everything correctly. If it doesn't get a number from any of those values it'll set the scale to 100%, regardless of what the scale actually should be (in your example it should be 320%).

I tried recreating your issue in jsfiddle: http://jsfiddle.net/ta27w58c/2/ It loads an image with crop data into a responsive canvas, which has a max-height set on it. The canvas and result image are inside a resizable div. Everything seems to function as expected on my end, so I'm not sure how to replicate your issue.

@tinquang
Copy link

Thanks for your quick support @iblank . In my case, there are few things that different from your test on jsfiddle: I turn on cropexif to auto rotate image, and I let user crop image on angular ui modal. Currently I sovled my problem by add 1 more function to cropHost factory and use it instead of your crop-data, but when I have time, I'll be back to test it again with crop-data, maybe there is something wrong on my side when apply your code.
Many thanks for your support.

@CallToPower
Copy link

@alexk111 Is this project dead or are you still active? Please have a look at some of the PRs, especially this one.

Fix issue with large images not rendering. Also, adds EXIF orientation detection back in, so if you experience cross-origin issues consider using previous commit.
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.