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

Angular Pattern, Redraw and Gulp Features #26

Closed
wants to merge 6 commits into from

Conversation

movibe
Copy link

@movibe movibe commented Nov 16, 2015

akreienbring and others added 6 commits May 28, 2015 22:47
Updated ionic version to 1.1.1
- add ng-annotate
- add gulp-strip-debug
- Refactored to John Papa Angular Pattern
(https://github.com/johnpapa/angular-styleguide)
- Redraw promise to real image size
# Conflicts:
#	dist/jr-crop.js
#	dist/jr-crop.min.js
#	package.json
#	src/jr-crop.js
@JrSchild
Copy link
Owner

Hi @movibe,

Thank you for you contribution. I have reviewed your PR and there are a couple of things that come to mind.

Regarding the scaling feature, I wrote this on a previous Pull Request:

When I initially created this library I didn't want to do this. I wanted to return a canvas element with the highest possible resolution based on the proportions. My motivation behind this was because the target size would not necessarily be the same size as the size on screen. Let's say you want to crop an image for a profile and you would like this in a higher resolution than the 200 by 200 pixels that you crop the image on screen in. Also across different platforms scaling an image could result in different quality files. Some browsers do this better than others. I wanted to leave it up to the developer to implement their own methods for downsizing the canvas element to the desired proportions.

I still stand by this thought, IMHO it's best to provide the image in the highest resolution possible. Whatever the developer does with it after, is up to them. I'm fairly sure performance is not great and resizing algorithms are not consistent therefore I'd rather leave that up to the server or your own client side implementation.

The 'updated' "gulp-sass": "^2.1.0" is not in devDependencies. I am missing explanation of decisions. Why the refactor? Why the Gulp features? What does void 0; do..? The template is nice, but headerHTML and footerHTML are nearly identical. I'll see what I can do with that, the problem is that it might break the existing API because template is now overwritable. So that might be something for the next major release. The option allowRotation should be false by default so the behavior doesn't suddenly change when updating.

Regardless, rotation seems like a good feature to have and I appreciate your and @akreienbring's work on this. I'll test that out and try to merge some of that code in soon. In the future, please try to keep the PR's small and exclude the /dist files, so everything can be reviewed and merged on a per-change basis.

@movibe movibe closed this Oct 21, 2019
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.

2 participants