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

fix(example app): use boolean values for FacebookAutoLogAppEventsEnabled and FacebookAdvertiserIDCollectionEnabled #565

Merged

Conversation

yaroslavnikiforov
Copy link
Contributor

Fixes #564

@yaroslavnikiforov yaroslavnikiforov force-pushed the fix-example-app-info-plist branch from 48dabba to c2a34ed Compare October 12, 2024 11:52
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This is a bit obscure, but this is the wrong place to change this 😆

sed -i -e 's/^<\/dict>/ <key>FacebookAutoLogAppEventsEnabled<\/key>\n <string>FALSE<\/string>\n<\/dict>/' ios/RNFBSDKExample/Info.plist
rm -f ios/RNFBSDKExample/Info.plist??
sed -i -e 's/^<\/dict>/ <key>FacebookAdvertiserIDCollectionEnabled<\/key>\n <string>FALSE<\/string>\n<\/dict>/' ios/RNFBSDKExample/Info.plist
rm -f ios/RNFBSDKExample/Info.plist??

Is where to change it, then you re-run refresh-example.sh

What!? Why? Keeping the example up to date is actually one of the most irritating burdens of being a react-native module maintainer, so I automate it as much as possible - and this is how, in this repo

I think it looks pretty easy to edit there though - that is the required changes are reasonably obvious I think? Can you make them in the script instead? Many thanks

@yaroslavnikiforov yaroslavnikiforov force-pushed the fix-example-app-info-plist branch from c2a34ed to e75aca8 Compare October 15, 2024 09:24
@yaroslavnikiforov
Copy link
Contributor Author

I updated my commit. But refresh-example.sh upgrades the react-native version and creates a bigger diff, should those changes be part of this PR?

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I think it's fine to leave the concrete example update to another commit - I usually do two commits, one to update refresh-example.sh and one that then has the result of re-running it

But I believe I ran this pretty recently so it should be fine, and if I did not run it + commit the result recently then there was a reason and it means I have something else to fix - which would be out of scope for this PR

TL;DR: this is great, your intended + desired change is now in the example and in the refresh script so it's permanent and that's really the point

Thanks again!

@mikehardy mikehardy added the pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. label Oct 15, 2024
@mikehardy mikehardy merged commit 1cfb158 into thebergamo:master Oct 15, 2024
4 checks passed
@mikehardy mikehardy removed the pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. label Oct 15, 2024
Copy link

github-actions bot commented Nov 6, 2024

🎉 This PR is included in version 13.1.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants