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

use react-native-image-picker for #3941 #4058

Closed
wants to merge 1 commit into from
Closed

use react-native-image-picker for #3941 #4058

wants to merge 1 commit into from

Conversation

aksonov
Copy link
Contributor

@aksonov aksonov commented Aug 23, 2019

@southerneer Feel free to finish - it is necessary to mock native module to pass ChatScreen test.

@southerneer
Copy link
Contributor

Is there a known issue with image-crop-picker that causes rotated images? Is it really necessary to pull in a new native dependency?

@aksonov
Copy link
Contributor Author

aksonov commented Aug 23, 2019 via email

@southerneer
Copy link
Contributor

southerneer commented Aug 23, 2019

I can't repro #3941. Android = Pixel 2, iPhone = iPhone XR. Sending from my Android to my iPhone all pictures look fine. @bengtan can you repro with 4.21.0? @aksonov which devices are you able to repro with?

@southerneer
Copy link
Contributor

I don’t understand the question.

What I meant was, is there a simpler solution to this problem? Why insert a whole new native dependency that does 90% of an existing native dependency? Did you try figuring out the issue in react-native-image-crop-picker? Something like this seems promising: ivpusic/react-native-image-crop-picker#379 (comment)

@aksonov
Copy link
Contributor Author

aksonov commented Aug 24, 2019

@aksonov which devices are you able to repro with?

Samsung S8 to iPhone SE

@bengtan
Copy link
Contributor

bengtan commented Aug 26, 2019

Quoting @southerneer:

can you repro with 4.21.0?

When I try to take a photo on my phone, the app (seemingly crashes and) restarts. I can't even take an image so I can't say whether it happens on my phone or not. I'm using a Nokia 5.

Quoting @aksonov:

Samsung S8 to iPhone SE

QA also uses a Samsung S-something to test with.

We're gonna run into this situation from time to time. Some bugs will only appear on Samsung phones. Some bugs won't happen on Samsung phones. Samsung's android is little bit weird. Since they are a large (majority?) part of the Android market, there will be cases when we test specifically for Samsung phones.

Having said that, it's not always Samsung which does special things. As mentioned above, the app restarts on my Nokia. There will be cases when the stock apps (in this case, the camera app) on an android phone do slightly non-standard things.


Hmmm ... maybe we should have a discussion about which native component (react-native-image-crop-picker vs react-native-image-picker) 'feels' better quality and has better support.

For example, if we switch to a different component due to a bug, but the new component isn't necessarily better quality overall, maybe it ends up being a net negative. (Just an example. I don't actually know how the two components compare.)

(As a Plan C, we can just de-prioritise this ticket.)

@southerneer
Copy link
Contributor

southerneer commented Aug 26, 2019

OK, I'm trying to restore an old Samsung S5 I still have, but it's taken literally 11 System Updates (and still counting). Hopefully once it's ready I'll be able to repro. Until then, I think we should explore simpler fixes before pulling in a new native dependency:

  • What happens if you apply cropping to the message photos. Does the rotation go away?
  • Maybe the rotation would be resolved by including exif data with includeExif in the request: https://github.com/ivpusic/react-native-image-crop-picker#request-object
  • Maybe we could experiment with other settings like compressImageMaxWidth and compressImageMaxHeight. The issue I linked to above was resolved in a PR that's included in our current version and it had to do with compression. Maybe if we compress the image the rotation will go away (?)

@southerneer
Copy link
Contributor

if we switch to a different component due to a bug

The thing is, this isn't a switch, it's an addition. We need crop-picker for cropping...rn-image-picker doesn't do cropping afaik.

@aksonov
Copy link
Contributor Author

aksonov commented Aug 26, 2019

@southerneer

  1. Cropping probably works well (because there are no complaints about bots/profiles/etc where we have cropping enabled)
  2. includeExif doesn't help
  3. I don't want to use compressImageMaxWidth, compressImageMaxHeight to avoid image resizing, but I found compressImageQuality as alternative and tried to set it to 0.8.. Also I found that guy within image-picker issue ([Android] Image rotated 90 degrees react-native-image-picker/react-native-image-picker#655) also suggested change compression as alternative for rotation: 360 trick. And.. it works well, I don't see rotation bug! Thanks for useful idea, it allows to avoid extra native dependency.

@aksonov aksonov closed this Aug 26, 2019
@aksonov aksonov deleted the 3941-2 branch August 26, 2019 13:50
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.

3 participants