Skip to content
This repository has been archived by the owner on Oct 24, 2022. It is now read-only.

Enable app events to be sent from the pixel for iOS and Android (#678) #732

Merged
merged 1 commit into from
Jan 17, 2019

Conversation

msencer
Copy link

@msencer msencer commented Jan 6, 2019

adding @peterpeterparker for the review :)

@peterpeterparker
Copy link
Collaborator

@msencer thx for the PR and for pinging me. First of all, it might take a while till merge because I don't have that much time, so sorry in advance for that, will try my best

That being said, I add a look quick look to the PR, could you plz do the following first modifications:

  1. In README, don't display the parameter --variable ENABLE_HYBRID_APP_EVENTS="true" as default example, this should be an option so don't displays it in the cmd line

  2. For consistency I think it would be good to rename ENABLE_HYBRID_APP_EVENTS to something like FACEBOOK_HYBRID_APP_EVENTS and enable_hybrid_app_eventsandfb_hybrid_app_events` etc.

  3. Add some null check to the code in case the parameter would not be defined at all (shit happens you know)

Furthermore could you plz:

  1. In the README document this feature respectively document what happens when activated, maybe with a reference to the Facebook doc

Finally:

  1. Could you provide plz a sample demo app to try it out?
  2. Did you check the backwards compatibility (just in case), does that work with for example cordova-android < v7?

Thx in advance

Context appContext = cordova.getActivity().getApplicationContext();
Resources res = appContext.getResources();
int enableHybridAppEventsId = res.getIdentifier("fb_hybrid_app_events", "bool", appContext.getPackageName());
boolean enableHybridAppEvents = enableHybridAppEventsId != 0 && res.getBoolean(enableHybridAppEventsId);
Copy link
Author

Choose a reason for hiding this comment

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

Add some null check to the code in case the parameter would not be defined at all (shit happens you know)

enableHybridAppEventsId != 0

part does that for android

@msencer
Copy link
Author

msencer commented Jan 17, 2019

@peterpeterparker thanks a lot for the quick review, sorry that it took so long for me to address the points. I did most of the modifications you've asked and have some comments/questions on the others:

  1. Add some null check to the code in case the parameter would not be defined at all (shit happens you know)

For Android; it is done by checking enableHybridAppEventsId != 0 and for iOS if the parameter is not defined *is_enabled becomes nil and the string comparison between nil and "true" returns false. That's why it should not be a problem.

  1. Could you provide plz a sample demo app to try it out?

I have the demo app working with my local fork, also added to the README.

  1. Did you check the backwards compatibility (just in case), does that work with for example cordova-android < v7?

It works only for cordova-ios >= 4.0.0 because of the WKWebView dependency. When I was testing for cordova-android v6 for instance, I came across with a similar problem to #705. I am pretty sure it is not related with the changes. Do you have any suggestions?

thanks for looking into this, very much appreciated!

@peterpeterparker peterpeterparker merged commit dcc02c3 into jeduan:master Jan 17, 2019
@peterpeterparker
Copy link
Collaborator

Thx for the PR, improvements, answers and demo app, really cool 👍

I've merged this PR and released it in version v4.1.0

I've tested my app (Android and iOS) without AppEvents (backwards compatibility ok) and your demo repo (AppEvents enabled) and all seems fine. I hope I could count on you @msencer in case it would need support later on 😉

Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
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.

2 participants