-
Notifications
You must be signed in to change notification settings - Fork 41
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
Supported swipe back when NavigationBar is hidden (iOS) #677
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Used notification center to raise event. The navigation bar listens for it and sets itself onto object. It's faster than searching subviews - especially when it might not even be there at all and have to check all subviews
If get the view form the controller then iOS loads the view. This is too early because React Native hasn't finished adding all the children. In particular can't find the navigation bar because it's not part of the subviews yet. So get scene directly instead of from controller
Added crumb to the find notification name so that only the right scene listens. So if there's a stack of 100 scenes only 1 listener fires. It will fire for at most one scene per stack. So if there's tabs it will fire for the matching crumb scene in each stack
Didn't seem to make a difference - it wasn't called even when not manually removing - but good pracitce
Using setHidden instead of setNavigationBarHidden allows the swipe back gesture to work. Good if someone wants to use a custom navigation bar - like Lindhout who's migrating from RNN. Allows him to migrate to the Navigation router stack and still use custom navigation bar
Might be people setting title but not having NavigationBar. V unlikely but want to ensure it's backward compatible - so safe to show the navigation bar if there's a title
Accessing self.view loads the view so viewDidLoad runs before React Native has built the tree. Found this current bug earlier just slipped up again
It's more consistent with android - and it's confusing to check title (v unlikely anyone using title without navigation because can't style it)
Otherwise it's nil when accessed by NVSceneController and falls over
The navigation bar is now visible on Fabric
Don't want to rely on navigation bar not being there because the status bar only works if the navigation bar is there (what if want not navigation bar, swipe back and status bar). Instead using the back title to decide which type of hide. Hidden with no back title is the default current behaviour. Hidden with back title keeps the swipe
Could, in theory, keep hidden true and toggle backTitle so can enable and disable the swipe. So need to apply different hidden settings inside the NavigaitonBar too. Changed property from hidden to isHidden because hidden is an iOS property already and it kept being set to true when toggling backTitle even if hidden prop stayed false
grahammendick
added a commit
that referenced
this pull request
Apr 1, 2023
Errors introduced from PR #677. These props were added for iOS so also needed on fabric
It works, thank you! |
Closed
grahammendick
added a commit
that referenced
this pull request
Mar 31, 2024
Used to be that when navigation bar was hidden the swipe back gesture was disabled. So put in #677 to hide navigation bar in a different way to keep the swipe back (turned this on when back title was non-empty). But now the NavigationStack implements UIGestureRecognizerDelegate, and returns YES from gestureRecognizerShouldBegin, the swipe back is automatically enabled even when the navigation bar is hidden. So backing out #677 (on new arch, will do old arch in later commit)
grahammendick
added a commit
that referenced
this pull request
Apr 12, 2024
Used to be that when navigation bar was hidden the swipe back gesture was disabled. So put in #677 to hide navigation bar in a different way to keep the swipe back (turned this on when back title was non-empty). But now the NavigationStack implements UIGestureRecognizerDelegate, and returns YES from gestureRecognizerShouldBegin, the swipe back is automatically enabled even when the navigation bar is hidden. So backing out #677 (on old arch, already done on new arch)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #664
@RichardLindhout is migrating from RNN to the Navigation router. Migrating all his custom navigation bars over to the Navigation router's native
NavigationBar
is too much in one go. So he wants to turn off the native navigation bar and use his own for now. But settinghidden
to true on theNavigationBar
prevents the swipe back gesture on iOS.There's a difference between
setNavigationBarHidden
onUINavigationController
andsetHidden
on theUINavigationBar
. The first disables the swipe but the second doesn't. I'm not sure if it's official iOS behaviour (it still works on iOS 16).If the
NavigationBar
is hidden checked forbackTitle
to decide which way to hide the navigation bar. If there's abackTitle
usedsetHidden
to keep the swipe.The original plan was to use the presence of the
NavigationBar
to determine whether to dis/able the swipe. But the Navigation router doesn’t support aStatusBar
without aNavigationBar
. Couldn’t hide the navigation bar, keep the swipe and customise the status bar.While investigating this original plan found a fast way to check for no
NavigationBar
at all using theNSNotificationCenter
. Firing a notification that theNavigationBar
receives avoids having to search through all descendant views.Even though the PR moved away from this plan, kept the
NSNotificationCenter
in place. Ever since Fabric stole tags, wanted a fast way to find theNavigationBar
(replacingviewWithTag
). For example, it’s possible to put theNavigationBar
at the bottom of the page (AndroidCoordinatorLayout
examples can put it at the bottom) which would’ve meant trawling through all views to find it.