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

Support resizing and cropping native images #7488

Merged
merged 12 commits into from Oct 6, 2016

Conversation

Projects
None yet
3 participants
@kevinsawicki
Contributor

kevinsawicki commented Oct 4, 2016

This pull requests add cropping and resizing support to native images.

  • Add resize method
  • Add crop method
  • Add getAspectRatio method
  • Add tests
  • Add docs

Closes #6523

@davej

This comment has been minimized.

Contributor

davej commented Oct 4, 2016

Awesome. 👍

(Something small I noticed… the type signature for nativeImage.getSize() is incorrect, it should be an Object. Do I just PR the english docs to fix it or are the type signatures now being generated from somewhere?)

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 4, 2016

Do I just PR the english docs to fix it or are the type signatures now being generated from somewhere?

PR to docs/api/native-image.md should be good 👍 Thanks.

@@ -215,4 +215,28 @@ Marks the image as a template image.
Returns `Boolean` - Whether the image is a template image.
#### `image.crop(region)`
* `region` Object - The region of the image to crop

This comment has been minimized.

@zcbenz

zcbenz Oct 5, 2016

Contributor

rect would be a better name, region is usually an union of rects.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Oct 5, 2016

👍

@davej

This comment has been minimized.

Contributor

davej commented Oct 5, 2016

Just a thought… if I do an image resize with one option:

image.resize({ width: 200 });

Then I would have probably expected the height to scale proportionately with the width (rather than stay the same). It's rare that you would actually want to skew the image, the more common case is that you want to scale the image to a particular width or height. Of course this can be done in userland using javascript and image.getSize() so it's not a big issue. Might be worth mentioning it in the docs though.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 5, 2016

Then I would have probably expected the height to scale proportionately with the width (rather than stay the same)

Good call, I've updated this pull request to now have that behavior. Setting only height or width will use a preserved aspect ratio value for the other.

Also added a getAspectRatio API since that might be useful as well when resizing/cropping.

@davej davej referenced this pull request Oct 5, 2016

Closed

Images with orientation error #4

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Oct 6, 2016

👍

@zcbenz zcbenz merged commit 10b91b1 into master Oct 6, 2016

8 of 9 checks passed

electron-win-ia32 Build #1617 failed in 8 min 5 sec
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #4340742 succeeded in 67s
Details
electron-linux-ia32 Build #4340743 succeeded in 58s
Details
electron-linux-x64 Build #4340744 succeeded in 113s
Details
electron-mas-x64 Build #2524 succeeded in 8 min 44 sec
Details
electron-osx-x64 Build #2529 succeeded in 8 min 35 sec
Details
electron-win-x64 Build #1592 succeeded in 8 min 11 sec
Details

@zcbenz zcbenz deleted the native-image-resize-and-crop branch Oct 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment