-
Notifications
You must be signed in to change notification settings - Fork 20
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
Ignore view and session lifecycle events when switching between tabs #50
Conversation
… of a visitable view controller.
This reverts commit f85d94e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks @svara!
Feature-wise, this is great! Fixes the bug entirely and doesn't introduce any regressions (that I could find). But I'm concerned about having to use a custom navigation controller. What happens when a developer uses a third-party one? I'm guessing they can subclass that then add the behavior we have in the Hotwire one. If that's the case, let's make sure to add documentation around that to the site. Perhaps a stretch, but is there any concern about tying this so closely to tabs? What about Split View controllers? What about completely custom stuff? Or is that out of scope and left to the developer? As I write this, even more reason to make sure this is documented. |
return | ||
} | ||
|
||
if visitable === topmostVisit.visitable && visitable.visitableViewController.isMovingToParent { | ||
// Back swipe gesture canceled | ||
if topmostVisit.state == .completed { | ||
currentVisit.cancel() | ||
} else { | ||
visit(visitable, action: .advance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment here? It's the only code path which doesn't have one and I'm not entirely sure what it covers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code path is hit when the back swipe gesture is cancelled.
We should probably move the comments out of if statements.
If a developer uses a custom navigation controller, they must now subclass
Fair points. As we found out, at the view controller level, there's no way to determine the exact reason it appeared or disappeared. With the proposed approach, developers would still be able to subclass our custom implementations. Any other completely custom setups would remain out of scope. |
All of that makes sense to me, thanks @svara! |
@joemasilotti I've documented |
LGTM! Getting the same on the site is a great idea. I think all that's left is addressing the comment in the |
This PR addresses the issue of pages being reloaded when switching between tabs - #40.
Context
Switching between tabs triggers the same view lifecycle events as when views are popped or pushed on a navigation controller. There is no way to distinguish whether a view appeared because its tab was selected or because it was pushed onto a navigation controller. Therefore, this PR introduces a custom UINavigationController subclass -
HotwireNavigationController
, which tracks and sets a view controller's appear and disappear reasons.Switching tabs should not alter the view hierarchy. That's why we guard against any changes when the appear and disappear reasons are due to a tab change.