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 modal navigation #85

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

leonvogt
Copy link
Contributor

This PR attempts to solve the issue #84.

Problems solved:

  • Modal closes after redirecting to /resume_historical_location
  • Jump to the first fragment, when a modal was closed on the second fragment via /recede_historical_location

One left inconsistency between Android and iOS is that:

  • After the modal is popped, through /recede_historical_location, Android requests the location of the previous fragment. iOS doesn't do a request.
    Seems to me, that happens because the navigator always request the location of the newly attached fragment in: HotwireWebFragmentDelegate.kt#328
    Not sure how to proceed here.

Another thing is, that the app previously freezed when the modal was popped through /recede_historical_location on the first fragment (as mentioned in the issue). I've fixed that by setting shouldNavigate to false, if we are popping a fragment.
But i guess it could be worth to investigate more, why the app freezed in the first place.

@leonvogt
Copy link
Contributor Author

I did not continue working on this PR since this comment in the iOS repo suggests that the behavior of historical_locations may change in the near future. (Like closing the modal, on /resume_historical_location)

Unless I hear otherwise, I'll wait for the changes in iOS before continuing work on this PR.

The only downside is that the current Android version will freeze when the server redirects to /recede_historical_location and a modal is present, as described in the issue.

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

Successfully merging this pull request may close these issues.

1 participant