Skip to content
This repository has been archived by the owner on Jun 16, 2023. It is now read-only.

Fixing ios issues with basic/example #2775

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rfischer
Copy link
Collaborator

@rfischer rfischer commented Apr 4, 2020

Summary

The basic example project wasn't working with the latest Xcode version. Therefore I made some changes:

Test Plan

I tested it by running the example ios project and checking whether changing the camera settings still works or not.

Compatibility

OS Implemented
iOS
Android

René Fischer added 3 commits April 4, 2020 08:21
- upgraded dependencies especially react-native to support Xcode 11 (https://github.com/facebook/react-native/pull/25146/files)
- texts of the top buttons are now readable
- ios: added NSMicrophoneUsageDescription to info.plist
- updated to recommended Xcode project settings
Added a method to lock a capture device, apply settings via a block, and unlock the device again.
@fabOnReact
Copy link
Contributor

fabOnReact commented Apr 4, 2020

Thanks for this pull request @rfischer

I reviewed again now some of your changes. You are upgrading react-native dependency and fixing this issue

https://github.com/facebook/react-native/pull/25146/files

Which I already experienced and work on #2735 (comment)

I need to test the changes on android, as I tried to upgrade the basic example myself in the previous weeks, but failed doing it..

I would still keep two separate pull request:

  1. first pull request - fixing the IOS Basic example
  2. second pull request - adding the changes you want to add to Custom White Balance

You can branch out of your second pull request and add those changes temporarely to test the basic example... That is how I am working now... until we have fully working development environment.

It is easier to work with small pull request.

Sorry for closing this before, I understand now that you are having some issues with the development environment, I have been trying to fix those issues in the past but then prioritized other tasks...

Let me test this changes on Android, make some changes to this pull request and I will give you an update tonight.

If you want to discuss or need help in chat, I am available full time to assist you

Thanks a lot
Fabrizio

@fabOnReact fabOnReact reopened this Apr 4, 2020
@fabOnReact fabOnReact changed the title iOS Refactoring upgrading basic example react-native dependency Apr 4, 2020
@fabOnReact fabOnReact changed the title upgrading basic example react-native dependency fixing ios issues with basic/example Apr 4, 2020
ios/RN/RNCamera.m Outdated Show resolved Hide resolved
@fabOnReact fabOnReact changed the title fixing ios issues with basic/example Fixing ios issues with basic/example Apr 4, 2020
@fabOnReact fabOnReact self-assigned this Apr 4, 2020
@fabOnReact fabOnReact linked an issue Apr 4, 2020 that may be closed by this pull request
Copy link
Contributor

@fabOnReact fabOnReact left a comment

Choose a reason for hiding this comment

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

I tested your changes on Android and everything works fine (even MLKIT features).
Really nice pull request, I hope we will merge also 2774 very soon.

Can you accept my changes published in pull request rfischer#2 ???

I tested using react-native version "0.59.9"

I just have one more doubt explained below.
I still need to test this branch on IOS.

Thanks a lot
I wish you a good weekend
Fabrizio

ios/RNCamera.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
@fabOnReact
Copy link
Contributor

I tested the pull request on both iPhone and Android development environment.

This pull request fixes the issues previously experienced in this comment #2735 (comment) so I believe it is a priority to merge it.

Closes Feature Request #2736 by upgrading the react-native version for the basic example to version higher then 0.59.1 (pull #25146).

I was able to install the project before and after deleting yarn.lock. I tested on Ubuntu and Mac os environment and bundling yarn works on both and does not create any diff.

I will remove mlkit feature from the basic example in my next pull request.

Thanks a lot @rfischer for fixing this, I tried to upgrade this example to 0.61.5 in the past.. but could not go through all the errors... This is really important pull request and was experienced from many users, I wish you best luck also for 2774.

Just a notice, we did some git revert of previous commits, I checked this branch and I did not notice any errors. I would still keep coming back to this pull request for 4-5 days to review it in case I forgot something.

Thanks a lot
I wish you a good weekend 😃
Fabrizio

@fabOnReact fabOnReact removed their assignment Apr 6, 2020
@@ -5,7 +5,7 @@
"@babel/runtime": "^7.1.2",
"babel": "^6.23.0",
"react": "16.8.3",
"react-native": "0.59.1"
"react-native": "^0.59.1"
Copy link
Contributor

@fabOnReact fabOnReact Apr 7, 2020

Choose a reason for hiding this comment

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

@rfischer the same changes needs to be done for /examples/mlkit/package.json. I am working on it, let me know if you want to add it to this pull request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fabriziobertoglio1987 I never tried the mlkit example so far. But if the same fix is also needed there, then you can add it to this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

fabOnReact@52ec1df otherwise I publish the pull request. You need to register in firebase and download the GoogleServices-Info.plist file, but don't mind this .. I will resolve this comment as soon as I publish the pull request.. Just a reminder that it has to be done. Thanks a lot
image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve basic example
2 participants