grahammendick / navigation

Scene-Based Navigation for React and React Native
https://grahammendick.github.io/navigation/
Apache License 2.0
590 stars 41 forks source link

Back Button never shows up on Android #590

Closed karlsander closed 2 years ago

karlsander commented 2 years ago

I'm rendering a fairly straightforward NavigationBar, and I would expect it to include a back button in cases where stateNavigator.canNavigateBack(1) returns true, but nothing shows up. Is it supposed to? Is it implemented on android, and if so, do I need to do anything special?

<NavigationBar
      hidden={false}
      hideOnScroll={false}
      tintColor={color}
      titleColor={isPassed ? color : "transparent"}
      barTintColor={Colors.background}
      title={title}
    >
      {children}
</NavigationBar>

The Navigation Bar always renders with the title at the very left, like on a screen that can't go back.

grahammendick commented 2 years ago

Hey there, you need to add a navigationImage, like in the Twitter sample, otherwise the back button won't show

<NavigationBar
  navigationImage={require('./arrow.png')}
  onNavigationPress={() => {
    stateNavigator.navigateBack(1)
  }} />
karlsander commented 2 years ago

Thank you! I should have checked the Twitter example, that already got me unstuck on the android architecture for a tabs based app.

I think having this behavior by default would still be good though, since it is the default on iOS and I would expect almost everyone wants it on android too? Is this just a backburner issue or is there maybe some complexity I'm not considering (as is often the case with these things)? I guess it would be a breaking change at that point, so maybe with a showBackButton prop?

karlsander commented 2 years ago

It does not, however, seem to work. The behavior did not change, either with a minimal version with just those two props, or with the following example that has everything I could think of thrown in. The require with absolute urls works verbatim like this as a tab image, so that shouldn't be the problem, right?

const { stateNavigator } = useContext(NavigationContext);
console.log("can navigate back", stateNavigator.canNavigateBack(1));

return (
    <NavigationBar
      key={title + stateNavigator.canNavigateBack(1)}
      hidden={false}
      backTitle="Back"
      navigationAccessibilityLabel="Back"
      hideOnScroll={false}
      tintColor={color}
      titleColor={isPassed ? color : "transparent"}
      barTintColor={Colors.background}
      title={title}
      navigationImage={
        stateNavigator.canNavigateBack(1)
          ? require("shared/assets/icons/k-tab-3.png")
          : undefined
      }
      onNavigationPress={
        stateNavigator.canNavigateBack(1)
          ? () => {
              stateNavigator.navigateBack(1);
            }
          : undefined
      }
    />
  );

The back button works (after I took the tab architecture from twitter example), so I think my breadcrumb setup is right. My root looks like this:

      <NavigationHandler stateNavigator={navigators.tabs}>
        <NavigationStack hidesTabBar={hidesTabBar} />
      </NavigationHandler>
grahammendick commented 2 years ago

To show a back button Android you only need a navigationImage prop. It's then up to you what happens in the onNavigationPress

<NavigationBar navigationImage={require('./arrow.png')} />

I'm a bit confused. Have you got it working by following the Twitter example?

I think having this behavior by default would still be good though

We did discuss it in the original NavigationBar on Android PR but decided against it. Have a read of the PR comments and let me know what you think.

karlsander commented 2 years ago

No, sorry that was a little confusing / premature. I don't have it working.

The twitter sample helped me with a totally unrelated issue that caused a bit of hair pulling before.

So the expected logic is "when a navigation image is passed, show the button in cases where it's possible to go back"?

grahammendick commented 2 years ago

No, the logic is "when a navigation image is passed show the navigation image". The NavigationBar doesn't care whether you can navigate back or not - it shows the image if you pass it.

karlsander commented 2 years ago

I'm sorry for being a little dense here. I really appreciate your quick replies and this entire (honestly really impressive) project. I guess I'll fiddle with the image require some more, if that's the only thing needed to show a button there.

We did https://github.com/grahammendick/navigation/pull/311#issuecomment-544409319 but https://github.com/grahammendick/navigation/pull/311#issuecomment-544416032. Have a read of the PR comments and let me know what you think

It makes some sense to me that you would often have a menu or something in that location on android instead. Still I think an implementation that is more "discoverable" would be nice. Something that you could possibly guess from looking at the component type. Having an "enableBackButton" prop that uses a system image, the "one level back" handler and automatically hides when back is not possible seems very appealing to me. I would have discovered that in 30 seconds. It may not be clean API design if there are two competing ways to configure the behavior? If it could also be made to work on iOS (disable back if it is false) it seems good.

I would expect that naively implementing an app using just the NavigationStack / NavigationBar / navigate, basically following the docs tutorial, should produce a simple, usable, shippable app on Android and iOS. Fully automatic back is already how it works on iOS and just about every stack based Android app I've tried works like this. To me it seems like obviously the right default and the PR discussion didn't really convince me otherwise.

karlsander commented 2 years ago

I have solved it: The culprit was indeed the image require. If I require something from the same folder, it works.

Summary for future readers: To enable a button in the back position on android, you need to pass a working image to navigationImage. You also need to implement onNavigationPress yourself to make it work. Like this:

const { stateNavigator } = useContext(NavigationContext);
// ...
<NavigationBar
  title="Title"
  navigationImage={require("./arrow.png")}
  onNavigationPress={() => stateNavigator.navigateBack(1)}
/>

It's quite possible that has something to do with the way expo does it's own thing with assets, which we can't do much about. I guess my main takeaway at this point would be that needing to pass a (working) image is not an intuitive way to enable the button. A docs contribution is the best I can do (because of lack of java knowledge) so I will tackle that/add it to my to-do list.

grahammendick commented 2 years ago

I'm glad you've got it working!

I've had a look back at the documentation and I can see your point. I don't think I even mention the navigationImage in the docs so I can understand the confusion. I'd really appreciate a contribution to the Navigation Bar doc.

Also, I'll have another think about whether it's something we add by default. I made a case for it in the original PR but was voted down ;).

grahammendick commented 2 years ago

I've documented the Android back button in the "Adding Buttons" section of the "Navigation Bar" doc.

grahammendick commented 2 months ago

Fully automatic back is already how it works on iOS and just about every stack based Android app I've tried works like this. To me it seems like obviously the right default and the PR discussion didn't really convince me otherwise.

@karlsander I just released automatic back navigation in navigation-react-natve v9.21.0. Now, if you don't add onNavigationPress then the NavigationBar will automatically add a back arrow and navigate back when pressed. It uses Android's in-built action bar support so it's 100% native.

Also, the NavigationBar automatically hooks into the the Navigation router's new Drawer component too. Again, if you don't add onNavigationPress, then it will add a hamburger icon to control the Drawer. Because it's 100% native you'll get Android's in-built hamburger-to-arrow toggle animation.

I thanked you in the release notes.