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

Allow TabBarBottom accept onTabPress callback (#486) #525

Closed
wants to merge 1 commit into from

Conversation

kimdhoe
Copy link

@kimdhoe kimdhoe commented Mar 1, 2017

Pressing a tab bar label often requires performing some kind of side effects
other than navigating: Scroll-to-top functionality, for instance.
At the moment TabView.TabBarTop, which is the default on Android,
handles onTabPress callback as expected, but TabView.TabBarBottom
ignores it.

This PR addresses the problem so that TabView.TabBarBottom can accept
and handle the optional onTabPress callback. The callback is invoked
exactly once if present on every tab bar press, accepting as a sole
parameter a NavigationRoute, which is the route object the pressed
target represents.

Fixes #486.

@satya164
Copy link
Member

satya164 commented Mar 1, 2017

I don't think this is the right way to do it. How would you wire it up to with the correct rendered navigator instance when you have multiple navigators mounted? Seems impossible with this approach.

@kimdhoe
Copy link
Author

kimdhoe commented Mar 2, 2017

@satya164 I'm thinking dispatching an action to a Redux store from onTabPress, so that child-screen can be passed relative information, which is a route's name in my use case.

@satya164
Copy link
Member

satya164 commented Mar 2, 2017

@kimdhoe yes, but say you have the navigator rendered in two places, for example it might just be tabs nested inside a stack, then how do you know which one sent the event?

@kimdhoe
Copy link
Author

kimdhoe commented Mar 2, 2017

@satya164 Hmm.. I thought I could use route names for that. Wouldn't it be possible if I use unique names for all screens?

@satya164
Copy link
Member

satya164 commented Mar 2, 2017

Route names are unique per navigator. If you have the same navigator rendered in two places, they will have same route names.

@kimdhoe
Copy link
Author

kimdhoe commented Mar 4, 2017

I hadn't thought that far ahead. It would be tricky to make a correct view to scroll to top in such complicated situations.

In Redux store or context, I'd have to keep track of route names and refs to ScrollViews in the last mounted tab navigator, which would be visible at a moment. The structure will be something like LIFO stack. Whenever a new tab navigator is mounted, new data will be pushed to the stack, and poped from it on unmounting. Then, when a route name is sent from onTabPress, a ref to the correct ScrollView could be retrieved from top of the stack.

But even if such fine control is impossible, IMHO adding onTabPress is useful since it can be used for general purposes.

@satya164
Copy link
Member

satya164 commented Mar 4, 2017

I'm sure that fine control is possible if the event listeners could be specified as part of screen configuration rather than being defined when navigator is defined. It's a common pattern and shouldn't require redux or complicated logic to achieve.

@stevehollaar
Copy link

stevehollaar commented Mar 6, 2017

I have a similar requirement, where I would like to dispatch a navigation action whenever the active screen's tab button is pressed.
I solved it in a similar way as this diff, but by passing the navigation instance, via this.props.navigation (which is available, but unused) in TabBarBottom.js to the onTabPress callback.

@satya164 I believe that would handle the ambiguous case of multiple Navigators, and passing navigation instances to callbacks seems to be a common pattern.

@kimdhoe
Copy link
Author

kimdhoe commented Mar 11, 2017

Ah, I see. I have come to something like this:

class RecentChatsScreen extends React.Component {
  static navigationOptions = {
    tabBar: () => {
      onTabPress: (scene: TabScene) => {
        if (scene.focused) {
          // scroll to top please
        }
      }
    }
  };
}

But I still had to manually manage a stack of ScrollView refs to implement scroll to top.

…ct-navigation#486)

Pressing a tab bar often requires some kind of side effects other than
navigation to be performed. This PR will allow a screen accept
`onTabPress` callback as an option. It will be invoked exactly once on
every press on the screen's corresponding tab button, receiving a
`TabScene` object as an argument.
@kimdhoe
Copy link
Author

kimdhoe commented Mar 12, 2017

Updated the PR. This will allow:

class RecentChatsScreen extends React.Component {
  static navigationOptions = {
    tabBar: {
      onTabPress: ({ route, index, focused }: TabScene) => {
        if (focused) {
          alert('active tab pressed.');
        }
      }
    }
  };

  // ...
}

yury added a commit to yury/react-navigation that referenced this pull request Mar 26, 2017
@Ingibjorg
Copy link

@satya164 can you review this again? I could really use this functionality in my app 💯

@gregorykan
Copy link

@satya164 i'd be keen for this too :)

@kennydl
Copy link

kennydl commented Mar 30, 2017

+1

Copy link

@leekiernan leekiernan left a comment

Choose a reason for hiding this comment

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

Is there anything holding this back? It seems a very reasonable implementation.

@justinmoon
Copy link

My use case is to have a tab icon that simply opens a modal (e.g. how camera is opened in Instagram).

For this use case, I'd need to be able to prevent the default jumpToIndex(index) call.

Thought I'd throw that out there.

@rad182
Copy link

rad182 commented Apr 12, 2017

+1

3 similar comments
@gislitg
Copy link

gislitg commented Apr 12, 2017

+1

@artembloom
Copy link

+1

@navid94
Copy link

navid94 commented Apr 21, 2017

+1

@jamesisaac jamesisaac mentioned this pull request Apr 21, 2017
9 tasks
@jamesisaac
Copy link

@kimdhoe Seeing as the proposed callback doesn't have access to this.props.navigation, how would you implement something like resetting the stack when the focused tab is pressed (common pattern)?

@cooperka
Copy link

cooperka commented May 2, 2017

Somehow I missed this when searching open PRs (searched onPress instead of onTabPress, d'oh!)...

I've implemented onPress consistently for both TabBarTop and TabBarBottom in this PR: #1335. Curious what your thoughts are. It's a bit DRYer and allows overriding the method to prevent the default action (because in some cases, you may not actually want the tab to change).

@spencercarli
Copy link
Member

Hey @kimdhoe thanks for putting the time and effort into this!

I'm going to close this in favor of #1335 since that PR is able to be merged immediately and has picked up some more feedback from the community.

Again, thank you for putting the time into this!

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.

None yet