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

Prompt error on iOS when camera access is disallowed #1036

Merged
merged 2 commits into from
Jan 4, 2021

Conversation

itsimon21
Copy link
Contributor

Details

Fixed Issues

When tapping "Take Photo", the app should request permission to access the camera.

Tests

Prerequisite: Allow Expensify.Cash to access: Camera is disabled in device settings

  1. Open the app on iOS and log in
  2. Tap on the text input field (bottom of screen)
  3. Tap the attachment icon (shown as a paperclip)
  4. Tap on "Take Photo"

Screenshots

@itsimon21 itsimon21 requested a review from a team as a code owner December 22, 2020 17:01
@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2020

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@botify botify requested review from robertjchen and removed request for a team December 22, 2020 17:01
@itsimon21
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@AndrewGable
Copy link
Contributor

recheck

2 similar comments
@itsimon21
Copy link
Contributor Author

recheck

@itsimon21
Copy link
Contributor Author

recheck

@robertjchen
Copy link
Contributor

recheck

@robertjchen
Copy link
Contributor

Thanks! I'll test this out locally myself and will merge after 👍

@@ -51,6 +52,24 @@ function showDocumentPicker(callback) {
function show(callback) {
RNImagePicker.showImagePicker(imagePickerOptions, (response) => {
if (response.error) {
if (response.error === 'Camera permissions not granted') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some additional context on the unconventional error check here:

Since we're using the deprecated 2.x.x react-native-image-picker module at the moment, we're forced to use the textual error message returned by the library.

The new 3.x.x version has a dedicated error code for permissions, which is a far cleaner solution.

@AndrewGable Are there any plans for us to upgrade react-native-image-picker, or should this solution be acceptable for now? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think updating to the latest would be great 👍

Copy link
Contributor

@robertjchen robertjchen left a comment

Choose a reason for hiding this comment

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

Looks good for now, thanks! Checking in with @AndrewGable regarding react-native-image-picker versioning before merging 👍

Copy link
Contributor

@robertjchen robertjchen left a comment

Choose a reason for hiding this comment

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

@itsimon21 Thanks for your patience! Per the discussion in the thread here: #1036 (comment), would you mind upgrading react-native-image-picker to 3.x.x and checking the error code for the permissions error rather than the error message? Thanks!

@itsimon21
Copy link
Contributor Author

@robertjchen The new 3.x.x version of react-native-image-picker has removed the "showImagePicker" method. According to
document, the "customButtons: [{name: 'Document', title: 'Choose Document'}]," option is not found, so the "Choose Document" function will be disabled after updating to the new 3.x.x version. Do we need to keep the "Choose Document" function?

Copy link
Contributor

@robertjchen robertjchen left a comment

Choose a reason for hiding this comment

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

@itsimon21 Hm, good point 🤔 Not sure why they deprecated that functionality in 3.x.x

That custom button is pretty crucial functionality- the alternative is to re-design the UI to accommodate a separate button for react-native-document-picker functionality (in addition to react-native-image-picker). Will merge for now 👍

@robertjchen robertjchen merged commit 5e47173 into Expensify:master Jan 4, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS - No error popup when user tries to take photo and camera access is disallowed
3 participants