expo / ex-navigation

Route-centric navigation for React Native
997 stars 201 forks source link

RFC: support navigationBar buttonConfig #236

Open chirag04 opened 7 years ago

chirag04 commented 7 years ago

Right now exnavigation has a concept of renderRight and renderLeft which can be used to render navbar buttons. This works great and pushes the responsibility onto the user to render whatever they want.

However, I think there is scope to make the experience better and define a buttonConfig on navbar that renders standard ios/android buttons to make it look closer to the platform navbar.

It could look something like:

navigationBar: {
    leftAction: { actionId, title, icon, },
    renderLeft: () => {} // called if  no leftAction given.
    rightActions: [ {actionId, title, icon }, {...} ],
    renderRight: () => {} // called if no rightActions given,
    onActionPress: (actionId, routeParams) => {}
}

rightActions can also collapse to an overflow icon. This is a high level RFC. We need think about if we are covering most cases in the actionConfig. eg: iconTintColor, toolbarAndroid on android etc.

cc @skevy @brentvatne @vonovak

vonovak commented 7 years ago

I would love to see the toolbarAndroid or the one that goes with vector-icons being used to render the navbar on Android by default.

Note that "If the toolbar has an only child, it will be displayed between the title and actions." - which is what I'm using when rendering a search field same as it is in the gmail app. For that use case, it would make sense to add another function, eg. renderCenter(). Something like that might be useful for ios too - how would one currently go about rendering a search input on ios? edit: renderTitle() can do that.

@chirag04 when you mention actionConfig you're talking about what's in the brackets? ie. leftAction: {actionConfig here}? Just to clarify. Obviously on iOS the rightActions length would be limited to, say, 2 while on Android there can be bunch of them.

Also, afaik exNavigation currently automatically provides a back button if not overridden. I suppose if neither of leftAction or renderLeft are included, a back button would be rendered.

Lastly, imho it would be beneficial if navigationBar was a function accepting scene's props. That way I could access them in the onActionPress or for example to have a dynamic scene title (which is now set once and done). It seems it would require these changes. Thoughts?

sibelius commented 7 years ago

@chirag04 I think the following code is a common pattern when using navigationBar renderLeft and renderRight. To use EventEmitter to emit an event when renderLeft or renderRight button was clicked. We could have reserved events that will be called when renderLeft/renderRight were clicked

class MyComp extends Component {
  static route = {
    navigationBar: {
      title: 'title',
      renderLeft: (state: ExNavigationState) => {
        const { config: { eventEmitter }  } = state;

        return (
          <TextMenuButton
            tintColor={state.getBarTintColor()}
            onPress={() => eventEmitter.emit('cancel')}
          >
            Cancel
          </TextMenuButton>
        )
      },
      renderRight: (state: ExNavigationState) => {
        const { config: { eventEmitter }  } = state;

        return (
          <TextMenuButton
            tintColor={state.getBarTintColor()}
            onPress={() => eventEmitter.emit('done')}
          >
            Done
          </TextMenuButton>
        );
      },
    },
    styles: {
      ...NavigationStyles.SlideVertical,
      gestures: null,
    },
  };

  _subscriptionDone: EventSubscription;
  _subscriptionCancel: EventSubscription;

  componentWillMount() {
    this._subscriptionDone = this.props.route.getEventEmitter().addListener('done', this._handleDone);
    this._subscriptionCancel = this.props.route.getEventEmitter().addListener('cancel', this._handleCancel);
  }

  componentWillUnmount() {
    this._subscriptionDone.remove();
    this._subscriptionCancel.remove();
  }

  _handleDone = () => {
    this.props.navigator.pop();
  }

  _handleCancel = () => {
    this.props.navigator.pop();
  }
}
chirag04 commented 7 years ago

@vonovak :

I would love to see the toolbarAndroid or the one that goes with vector-icons being used to render the navbar on Android by default.

Not sure if this is too much coupling. toolbarAndroid from vector-icons lib or stock toolbar from RN or a plain view that does overflow icon is fine too.

Note that "If the toolbar has an only child, it will be displayed between the title and actions." - which is what I'm using when rendering a search field same as it is in the gmail app. For that use case, it would make sense to add another function, eg. renderCenter(). Something like that might be useful for ios too - how would one currently go about rendering a search input on ios?

idk about this. haven't thought of this.

@chirag04 when you mention actionConfig you're talking about what's in the brackets? ie. leftAction: {actionConfig here}? Just to clarify. Obviously on iOS the rightActions length would be limited to, say, 2 while on Android there can be bunch of them.

yes, actionConfig is the object you pass to leftActions. No need to limit really. gmail on ios has more than three icons. Let the user device here i guess.

Also, afaik exNavigation currently automatically provides a back button if not overridden. I suppose if neither of leftAction or renderLeft are included, a back button would be rendered.

Yes, if no leftAction and renderLeft, it will be empty or back button depending on the stack size.

Lastly, imho it would be beneficial if navigationBar was a function accepting scene's props. That way I could access them in the onActionPress or for example to have a dynamic scene title (which is now set once and done). It seems it would require these changes. Thoughts?

dynamic scene titles still work. you can use the title property on navbar which is a function with route params as argument. onActionPress will have a similar signature so you can use that for dynamic things. no need to make navigationBar itself as a function.

@sibelius :

I think the following code is a common pattern when using navigationBar renderLeft and renderRight. To use EventEmitter to emit an event when renderLeft or renderRight button was clicked. We could have reserved events that will be called when renderLeft/renderRight were clicked

Yup, that's how i have it setup in my apps. That works well on ios. But the same logic does not scale very well on android and when you want to share that config on ios and android. Overflow icon being one more problem. Also as a navigation lib, we can own what you have as TextMenuButton.

vonovak commented 7 years ago

I guess there's no need for renderCenter if there's renderTitle already

vonovak commented 7 years ago

Hi! I've been playing with customizing the navbar for a bit and here's a summary of my observations:

That being said, I did get it working with ToolbarAndroid, including the searchinput (gmail-like search) and all the actions changing dynamically based on some state, but it's not totally pretty :|

chirag04 commented 7 years ago

also cc @oblador.

DomiR commented 7 years ago

This still works, using the native android menu: https://github.com/facebook/react-native/issues/3004#issuecomment-202598006