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

Remove go_router dependency for navigation #1613

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Nov 27, 2024

Pull Request Description

This PR removes our dependency on go_router, and migrates our existing usage of go_router with the built in Navigator. There's a few reasons for this refactor:

  • We currently use a mix of go_router and Navigator. Since we are only using go_router for the Settings page, it seems a bit redundant to have the package when we could implement the same logic using Navigator. Thus, it makes more sense to use Navigator unless we require a specific feature from go_router (e.g., ShellRoutes)
  • Using only one of Navigator / go_router allows us to simplify state restoration later on (e.g., when the app is killed by the OS to free up memory). Navigator has a simple method of creating restorable states whereas go_router is a bit more complex (and may not fully work well in some cases)

This is still in draft as there's a bit more testing / refining that is needed:

  • Testing deep links on Android / iOS (tapping a link from outside Thunder to open in Thunder)
  • DONE: Testing of routing within the Settings page to ensure all routing is properly handled (also, checking to see if Thunder-setting links open up properly Add ability to create links to settings. #1348)

Issue Being Fixed

This tangentially relates to, and is a first step to hopefully solving #1607. There is still a lot of additional steps that is required, but being able to restore the navigation state is the first step (e.g., to be able to navigate back to the CreatePostPage)

Issue Number: N/A

Screenshots / Recordings

Checklist

  • If a new package was added, did you ensure it uses an appropriate license and is actively maintained?
  • Did you use localized strings (and added appropriate descriptions) where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu hjiangsu force-pushed the refactor/remove-go-router branch from 9d24f7c to b8dbe27 Compare December 6, 2024 05:31
@hjiangsu hjiangsu marked this pull request as ready for review December 6, 2024 05:45
@hjiangsu
Copy link
Member Author

hjiangsu commented Dec 6, 2024

Update: I found the issue and applied the fix here since it seems fairly small - this is the commit which fixes the issue (at least on iOS): 1f7e2ba

Original comment:
So after some testing, it turns out that deep links does not seem to work in the current build on iOS (both develop and this branch).

@micahmo Could you check if deep linking works on the current develop for Android? If it's not working on Android either, I'll open up a new PR for that issue rather than to place the fix here.

Aside from that, I've tested everything else and it should be working so this is ready to go!

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

The code LGTM! Did you still need me to test anything in particular?

@micahmo
Copy link
Member

micahmo commented Dec 6, 2024

@micahmo Could you check if deep linking works on the current develop for Android?

Yes deep links are still working fine in develop!

@hjiangsu hjiangsu merged commit b5f5189 into develop Dec 6, 2024
1 check passed
@hjiangsu hjiangsu deleted the refactor/remove-go-router branch December 6, 2024 17:20
@hjiangsu hjiangsu mentioned this pull request Jan 17, 2025
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.

2 participants