Skip to content

Added 0.40 compatibility and updated usage in README#13

Open
syousif94 wants to merge 13 commits intomeznaric:masterfrom
syousif94:master
Open

Added 0.40 compatibility and updated usage in README#13
syousif94 wants to merge 13 commits intomeznaric:masterfrom
syousif94:master

Conversation

@syousif94
Copy link
Copy Markdown

@syousif94 syousif94 commented Mar 25, 2017

I updated the headers for react-native 0.40 and updated the JS in the usage instructions.

@meznaric
Copy link
Copy Markdown
Owner

Thanks for PR. I somehow missed it. I will merge it in next couple of days.


- (void)handleImageLoad:(UIImage *)image {

// hardcoded downsample of image
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want that? I think it makes more sense to keep original size.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi sorry, I did not realize further commits would modify this pull request. I think it would be helpful to let users set a max resolution though because loading a cropped full resolution photo from the camera album has a noticeable delay on my 6S. I can add that feature or just close this pull request.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good feature to have especially if delay is noticeable. Did you think about JS API already?

So far we have:

.cropImageWithUrlAndAspect(imageUrl, aspectRatio)
.cropImageWithUrl(originalImage.uri)

without breaking backwards compatibility we could introduce something like
.cropImage(originalImage.uri, options)
where options would be:

{
    maxWidth: 150, // optional, default no max width
    aspectRatio: ReactNativeImageCropping.AspectRatioSquare, // optional, default not locked
}

It would be easy to extend as well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sounds good, I'll update the pull request over the weekend.

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