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

feat(app): add Play Services available utilities #3601

Merged
merged 15 commits into from
Jun 3, 2020

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented May 4, 2020

fixes this issue: https://invertase.canny.io/react-native-firebase/p/playservicesavailability.

Replaces: #3240

NOTES

  • No tests for firebase.utils().makePlayServicesAvailable() or firebase.utils().resolutionForPlayServices() as I'm not sure it's possible. Will update if there is a way.

mikehardy
mikehardy previously approved these changes May 4, 2020
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.

Looks like exactly how I handle this in my app but with custom stuff, will be nice to have this in firebase!

Side comment - out of the 8 files changed, 5 were changed in non-semantic / formatting only ways, is that expected behavior? Makes the diff really big and a bit scattered when functionally it's not so large. Not a big deal, just something I noticed

@russellwheatley
Copy link
Member Author

Side comment - out of the 8 files changed, 5 were changed in non-semantic / formatting only ways, is that expected behavior? Makes the diff really big and a bit scattered when functionally it's not so large. Not a big deal, just something I noticed

@mikehardy It should've probably been in its own PR to be honest, but I ran the format:markdown script which formatted those files.

@Salakar
Copy link
Contributor

Salakar commented May 13, 2020

Side comment - out of the 8 files changed, 5 were changed in non-semantic / formatting only ways, is that expected behavior? Makes the diff really big and a bit scattered when functionally it's not so large. Not a big deal, just something I noticed

@mikehardy It should've probably been in its own PR to be honest, but I ran the format:markdown script which formatted those files.

Sorted on master, so this should be clean now.


In terms of review - I noticed @russellwheatley that the documentation has been done in the messaging package for some reason? Shouldn't be in there

Copy link
Contributor

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

See comment re messaging docs.

@russellwheatley
Copy link
Member Author

@Salakar I have no idea what that is doing in the messaging docs, prolly should've had a bigger slurp of coffee before that commit 😓 .

@russellwheatley russellwheatley marked this pull request as ready for review May 27, 2020 07:37
@russellwheatley russellwheatley requested review from Salakar and Ehesp May 27, 2020 07:37
@russellwheatley russellwheatley requested a review from Salakar June 2, 2020 11:57
@Salakar Salakar changed the title feat(app): check Play Services is available for Android feat(app): add Play Services available utilities Jun 3, 2020
@Salakar Salakar merged commit 0b0f858 into master Jun 3, 2020
@Salakar Salakar deleted the @russell/play-service-utils branch June 3, 2020 11:39
stefkampen pushed a commit to stefkampen/react-native-firebase that referenced this pull request Jun 6, 2020
mikehardy added a commit to mikehardy/rnfbdemo that referenced this pull request Jul 15, 2020
The correct fix would re-use invertase built-in gradle variable machinery
I just hacked in the "standard" 'getExt' function from most react-native-packages

This is a workaround for:
invertase/react-native-firebase#3943

Related to
invertase/react-native-firebase#3601
andersondanilo pushed a commit to vixtech/react-native-firebase that referenced this pull request Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants