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

[IOS][LINKS] Apply new restorationHandler implementation #1778

Closed
wants to merge 1 commit into from
Closed

[IOS][LINKS] Apply new restorationHandler implementation #1778

wants to merge 1 commit into from

Conversation

xzilja
Copy link
Contributor

@xzilja xzilja commented Dec 20, 2018

Summary

@Salakar this is universal linking issue we talked about with you earlier. Same one I opened react-native issue for facebook/react-native#22716

I basically applied same implementation as apple has documented here https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1623072-application?language=objc

And this fixed issues I was having in XCode 10 / ios 12. I tested this on a real device. I was not able to test with earlier iOS versions, but judging by apple's documentation this should be supported by iOS 8+ and previous implementation used Any instead of UIUserActivityRestoring, so there shouldn't be any issues here as well. And finally restorationHandler is not used as far as I can tell, it just needs to be passed in.

Checklist

Test Plan

Implement the change, check if it works on iOS devices that are running versions lower than 12. Has to be ran on a real device due to nature of simulator.

Release Plan

[IOS][ENHANCEMENT] [LINKS] - Updated universal link handling implementation for iOS 12


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@CLAassistant
Copy link

CLAassistant commented Dec 20, 2018

CLA assistant check
All committers have signed the CLA.

@xzilja xzilja changed the title Apply new restorationHandler implementation [IOS][LINKS] Apply new restorationHandler implementation Dec 20, 2018
@codecov
Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #1778 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1778   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          72       72           
  Lines        1838     1838           
=======================================
  Hits         1400     1400           
  Misses        438      438

3 similar comments
@codecov
Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #1778 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1778   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          72       72           
  Lines        1838     1838           
=======================================
  Hits         1400     1400           
  Misses        438      438

@codecov
Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #1778 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1778   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          72       72           
  Lines        1838     1838           
=======================================
  Hits         1400     1400           
  Misses        438      438

@codecov
Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #1778 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1778   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          72       72           
  Lines        1838     1838           
=======================================
  Hits         1400     1400           
  Misses        438      438

@xzilja
Copy link
Contributor Author

xzilja commented Dec 23, 2018

Related PR in react-native facebook/react-native#22764

@Salakar Salakar self-assigned this Jan 2, 2019
@Salakar Salakar added help: ios Needs help implementing or reviewing a PR relating to iOS code. platform: ios plugin: links Firebase Dynamic Links labels Jan 2, 2019
@Vanclief
Copy link

Vanclief commented Jan 9, 2019

Will this be merged soon? Experiencing same issue, @iljadaderko will be our hero if we can get this working.

@mikehardy
Copy link
Collaborator

Hi there! I'm helping maintain the v5 branch along with @Salakar and I believe we have CI all sorted and tests green etc so the v5 branch can tolerate change now. If you rebase to current v5.x.x and push this again, I can check into it and try to get it merged for you. @iljadaderko what do you think?

@xzilja
Copy link
Contributor Author

xzilja commented Jun 7, 2019

@mikehardy 👍 I completely forgot about this PR. I'll likely close it tomorrow and will submit new PR with few changes that are needed to support Swift (similar to one I submitted for rn Linking facebook/react-native#22764)

@mikehardy
Copy link
Collaborator

works for me - thanks!

@xzilja
Copy link
Contributor Author

xzilja commented Jun 8, 2019

Closing in favour of #2216 cc @mikehardy

@xzilja xzilja closed this Jun 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help: ios Needs help implementing or reviewing a PR relating to iOS code. platform: ios plugin: links Firebase Dynamic Links
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants