-
Notifications
You must be signed in to change notification settings - Fork 509
Conversation
I can test this. But unsute how to update my app with your PR. Can you provide me with a link to your repo? |
@ThorvaldAagaar sure
|
Thanks @peterpeterparker -- Plan on testing this out tomorrow. Maybe you should just ask @jeduan for maintainer status to get this in and put up a README note for Maintainers Wanted? |
cool @booleanbetrayal Regarding the fact, like I said, that I'm not a pro in this type of PR, I would definitely feel more comfortable about this if a maximum of users test it ;) About the maintainers wanted, sure I guess it would be cool if jeduan add a note in the README about that |
@peterpeterparker added you as collaborator on this project. Let me know your npm username so you can publish new versions. |
@jeduan I feel honored, not sure I have enough know-how for this so I will be really careful my npm username is peterpeterparker too |
I have published a post on the Ionic forum to try to find some more testers. https://forum.ionicframework.com/t/need-testers-for-cordova-plugin-facebook4-pr The more we test this on different phones and different apps, the better it is |
I'm seeing auth work as expected (with the new confirm modal) on an iPhone-6s iOS 11 physical device. Seems like the confirm is presented each time, which is a bit obnoxious. |
@booleanbetrayal thx for testing this PR 👍 What do you mean with "Seems like the confirm is presented each time", except the "new confirm modal" did you noticed something different? could you display that with a screenshot? If you are speaking about the "new confirm modal", like you, I'm not a big fan, I think that makes too much modals |
Yeah, I just meant that I'm not a big fan of that new modal in the flow @peterpeterparker . My experience sounds consistent with the intended behavior. So everything LGTM from what I can tell with this PR. |
@booleanbetrayal thx for the feedback, all clear then. I think it's good to collect a couple of more tests before merging this, I really don't want to screw something about the new modal flow I'm with you, I don't like it neither. I don't know what we could do, maybe nothing. Maybe an alternative would be now to implement a 100% web Facebook login based? don't know if that could be a good solution...should analyze this |
@peterpeterparker Added you to npm. I think you shouldn't do anything related ugly modals. Fighting people who want a different modal from Facebook is hugely draining and there are better ways to spend your time. Re: testing, people generally don't upgrade that often (requires rebuilding the app) so if you have a couple of tests, you should go ahead and publish it. |
Regarding cocoapods, I think you should do it. I tried a few ways, including moving the SDK to a separate npm module, Carthage and git submodules with a post-install script but nothing worked when this plugin was created. If cocoapods is supported now, go for it. |
tl;dr: Cocoapods dependencies on cordova work well. @jeduan @peterpeterparker The push plugin successfully uses cocoapods dependencies to pull the FCM code on the iOS side of things. It works, even thought the installation process and the project management on xCode is different. |
I'm a bit under pressure but as soon as I find time I'll publish this PR (I'll bump up version of the plugin to 1.10.0) About cocoapods, I was thinking about the example of push plugin too. I'll open a feature issue so we will still think about it |
This has been now release to npm Plugin version v1.10.0 @booleanbetrayal @jeduan iOS 11.3 improve the situation with the modal, there isn't anymore "two modals" and the login flow looks even clean |
For the record, an update of my app including the last update of the plugin aka including this Facebook SDK for iOS has been published in store respectively successfully reviewed by Apple Therefore, all good |
* Update FacebookSDKs-iOS-4.31.1 * contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9 * Update README according deprecation
* Update FacebookSDKs-iOS-4.31.1 * contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9 * Update README according deprecation
* Update FacebookSDKs-iOS-4.31.1 * contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9 * Update README according deprecation
* Update FacebookSDKs-iOS-4.31.1 * contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9 * Update README according deprecation
The following PR follows the reported issue #631
Context
Since a couple of days, Facebook displays a warning in their developers console in order to ask them to upgrade their
Facebook iOS SDK
What does that PR contains
This PR contains an update of the Facebook SDK
4.31.1
which could be found at the following address https://developers.facebook.com/docs/ios/componentsdks/I have updated the
showDialog
methodFacebookConnectPlugin.m
, respectively removed the code regardingcontentTitle/imageURL/contentDescription
because these are now marked as readOnly by Facebook / where deprecated in Facebook APIv.2.9
I have updated the
README
to add a notice regarding these deprecation in the chaptershare
What did I test
Using my app (productive app published in stores) I was able to succesfully
a. Build the plugin
b. Test both the
login process
andshare process
with a test version of my app including this PR on my test phone (iPhone 6s / iOS 11.2.6)c. Test the
login process
with a test version of my app including this PR in a simulated iPhone (iPhone 6 / iOS 9.1)Note regarding Login process and new popup on iOS 11
It look like that the this new Facebook SDK introduce a new additional popup to the login process with Facebook with iOS 11
Before this SDK:
Button click in my app -> Facebook www opened -> click -> popup to go to FB -> FB -> Accept terms -> Back in app
New with this SDK:
Button click in my app -> NEW POPUP -> Facebook www opened -> click -> popup to go to FB -> FB -> Accept terms -> Back in app
This new popup ask the user if he/she's agree to go from the app to Facebook www
Side note
I went manually thru all differences of all declarations files (
.h
files) and except the above deprecated variables I didn't noticed any major changes respectively no methods or variables or parameters where removedFinal note
I'm definitely no
Cordova plugin
or neitherObjective-C
expert, tried my best. Plz be careful while merging the PR.Final thought
For the future, maybe instead of being included in the plugin itself, shouldn't be the Facebook library be included thru CocoaPods? Just an idea...