search

hippware / rn-chat

MIT License
5 stars 0 forks source link

Estimate effort needed for custom image picker #1064

Closed southerneer closed 7 years ago

southerneer commented 7 years ago

Based on findings from #966 we need to do some more work on image picker functionality in the app. There a couple quick workarounds (enumerated on #966) that fall short of the ideal scenario.

@aksonov we want to know how much work it would take to enable the Square photo option seen on the native iOS Camera app. The idea is that we would use this option in the current app flow instead of making a user snap a picture then crop it to a square. Probably this would mean combining the best parts of the 2 RN libraries we've tried so far and adding the square option...

Requirements for the ideal solution:

Some (hopefully) helpful research material:

cc @zavreb @thescurry

thescurry commented 7 years ago

Thanks @southerneer

For the sake of clarity, we'd like the default for taking a photo to be a 1:1 square. Currently, the camera takes photos in 'portrait' mode and then the user has to crop the photo to 1:1... we'd like to remove any unnecessary steps by taking all photos in 1:1 square mode by default.

aksonov commented 7 years ago

The task was quite easy - it is not about native implementation of custom picker, but just definition of proper overlayView. There are several libraries that supports overlay - react-native-camera, react-native-camera-ios and react-native-camera-kit. So we need to 'mimic' iOS camera buttons, icons, everything to include into overlay view and define 1:1 'window' for camera. Luckily, the component react-native-camera-kit does all this job - so all we need is to pass '1:1' ratio and define overlayColor ('black' in our case). It didn't worked initially, but I forked and fixed the bug.

First draft is ready, I will do more testing by Monday. @thescurry Steve, if you want, please ask Eric to compile my PR and show you on a device (I can't codepush it because it is iOS-side).

southerneer commented 7 years ago

Sweet! I'll demo it for Steve today.

zavreb commented 7 years ago

@southerneer is it ready for demo?

southerneer commented 7 years ago

I hit a few snags testing the new camera screen on my phone (5s) and the test phone (6). I sent feedback to @aksonov . I'll check Slack by my 8am tomorrow which should give us time to work through any issues "live" before I come in tomorrow and can hopefully give a good demo then.

aksonov commented 7 years ago

I can't reproduce. Maybe you disallowed camera/photo gallery usage before? Anyway I've just added permission checks and going to deploy latest master to Staging to verify (debug mode on device works fine on my iPhone SE)

cc @bengtan

aksonov commented 7 years ago

One problem I found - camera always saves to camera roll, created github issue: https://github.com/wix/react-native-camera-kit/issues/62

mstidham commented 7 years ago

@aksonov I love the fact that camera saves to camera roll!
cc @zavreb

thescurry commented 7 years ago

I think the camera saving to the roll is advantageous since sometimes saving a bot/photo fails. This would allow for a user to retry without having to take another photo. Thoughts?

southerneer commented 7 years ago

I also think it's more of a feature than a bug to save to camera roll by default. If we do go this route I think we should think through the timing of prompts for camera + photo access (2 separate popups). As it is now prompting for camera when they take a picture makes sense but prompting for photo just after (when it saves to the roll) might be a little disorienting. Maybe asking for both up front would be slightly better?

southerneer commented 7 years ago

@aksonov the 1.43.1 release works fine on my 5s. I pulled the latest from Github and I'm running it in debug mode on the same phone and it's extremely slow initially but once the app has had time to complete all the initial queries I'm able to take a photo just fine.

zavreb commented 7 years ago

@aksonov, @thescurry, @southerneer is there a reason we decided to disable the flow that asked the user if they would want to 'Use Photo' v. 'Retake Photo' ?

cc: @thescurry to determine if we should reimplement or move fwd.

Also my 'Choose from library' image still rotates. @mstidham can confirm same issue.

zavreb commented 7 years ago

1088

aksonov commented 7 years ago

Continued investigation today. 'fixOrientation' method from react-native-image-picker mentioned by Eric didn't work - image-picker uses built-in camera and standard ios didFinishPickingMediaWithInfo callback, but we have to use own camera overlay and process photo with own logic. This bug happens only when we locked orientation (to portrait) like we did for TinyRobot. Created react-native-camera-kit issue: https://github.com/wix/react-native-camera-kit/issues/65 After more digging I found that react-native-camera module correctly handles it (verified on their Example) after that PR https://github.com/lwansbrough/react-native-camera/issues/227. I was correct that it is something with React-Native. It stops listening for rotation events if location is locked (but it still should do it for camera!).

So we have three ways:

  1. wait until react-native-camera-kit guys fix this bug (looks like they fixed it for Android already),
  2. fix it by ourselves or
  3. implement square overlay, cropping, retake using react-native-camera. Last two has about equal complexity, about 5-8.
thescurry commented 7 years ago

Thanks for the update @aksonov, it looks like that ticket is active. Do you have a gut feeling on when they may address this? If it's something they may take care of short term (next couple of weeks), let's wait. If not... we need to decide how to prioritize.

aksonov commented 7 years ago

It is unknown, as I see from other components, wix team could not work on issues for months. Anyway, I will ask them about ETA.

southerneer commented 7 years ago

Moving to "Waiting for Deploy" in favor of #1094

cc @zavreb

mstidham commented 7 years ago

@zavreb to verify

zavreb commented 7 years ago

For @bengtan and @thescurry to resolve.

zavreb commented 7 years ago

@thescurry and @bengtan where are we with this?

bengtan commented 7 years ago

@zavreb:

I'm not very sure.

I think ...

"Eric's rotation issue" is now part of #1094 ... which itself was supposed to be split into two (?) tickets.

"Pavel's rotation issue" still exists, I think, and the last I remember is that it needed an extra day-ish to fix but it wasn't high priority. (I'm not sure that this one has a ticket.)

(cc @southerneer, @aksonov)

So ... I'm guessing one way forward ... is to split #1094, create a separate ticket for Pavel's rotation issue, and then we can close this issue.

What do you guys think?

southerneer commented 7 years ago

sounds good to me

aksonov commented 7 years ago

it is ok

zavreb commented 7 years ago

Closing this one per @bengtan's comments.