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

Configuring screen options based on props and state #24

Closed
satya164 opened this issue Feb 15, 2018 · 32 comments
Closed

Configuring screen options based on props and state #24

satya164 opened this issue Feb 15, 2018 · 32 comments

Comments

@satya164
Copy link
Member

satya164 commented Feb 15, 2018

Currently the screen options can be specified statically. If you need to configure any options based on props and state of the component, or want to update state and props based on some action such as tab press, you need to do it in a hacky way by changing params. it's way more complicated than it needs to be. It also breaks when used with HOCs which don't hoist static props, a common source of confusion. (#145, #2725, #2804, #2955)

Proposal

We can add an additional method setOptions on the navigation prop:

class Home extends Component {
  constructor(props) {
    super(props);

    this.props.navigation.setOptions({
      title: 'Home'
    });
  }

  render() {
    ...
  }
}

If you call setOptions again at some point, the object is shallow merged with previous configuration. It might seem a little ugly at first, but this imperative API allows us to build a declarative API on top of it:

function Profile({ route }) {
  return (
    <React.Fragment>
      <NavigationOptions
        title={`${route.user}'s Profile`}
      />
      <ScreenContent />
    </React.Fragment>
  );
}

NavigationOptions is a React component that can be used to configure the current screen from the render method. It's very useful when your screen's configuration depends on the props, state or both. (#2546)

The static way of specifying the config can still be supported for simplicity and backward compatibility.

@dantman
Copy link

dantman commented Feb 15, 2018

If you need to configure any options based on props and state of the component, or want to update state and props based on some action such as tab press, you need to do it in a hacky way by changing params.

There's nothing hacky about communicating with params, params are basically props/state for the header.

it's way more complicated than it needs to be.

It's not more complicated than it needs to be. navigationOptions are static because some of them need to be read statically. For instance title/drawerLabel/drawerIcon/drawerLockMode are read by the DrawerNavigator to populate the drawer for drawer items corresponding to routes that are not even being rendered. DrawerNavigator only renders the active item, so none of the other instances have an instance you can use this.props.navigation.setOptions({}) or <NavigationOptions> in.

It also breaks when used with HOCs which don't hoist static props, a common source of confusion.

navigationOptions must be static, so we can't move the options to the instance. But perhaps if HOCs are a big issue we could switch the recommendation to an alternative to static navigationOptions = {}; that has less issue with HOCs.

Since HOCs are often applied using a decorator, how about a navigationOptions decorator.

@navigationOptions(({navigation: {params}}) => ({
  title: `${params.user}'s Profile`,
}))
@myHOC
class MyRoute extends PureComponent {
  // ...
}

navigationOptions are still static, they are still kept near the top of the class definition, and all we have to worry about is a "Make sure @navigationOptions is the top decorator in the list" warning instead of worrying about whether any one of the HOCs do not hoist static props.

@satya164
Copy link
Member Author

satya164 commented Feb 15, 2018

There's nothing hacky about communicating with params, params are basically props/state for the header.

Yes, params are props for the route.

It's hacky because of the way we need to use them to achieve basic things.

  1. If you need to trigger a side effect in the screen from an action in the header or vice-versa, for example update scroll position on a button click, you need to add something like a counter in the params which you increment and do stuff when that changes. Side-effects should not be done by doing things like updating a counter in the state.
  2. If you need share state between component and header, for example to show something in the header based on the screen's local state, you need to update params everytime your component state changes, now you have 2 sources of truth.

I have seen people pass instance methods, and sometimes entire component because it's too hard to do basic things with the current API. #145 is a great example of several things people are doing because of the limitations. Take a look at that and tell me it's not hacky.

It probably seems easy for you because you use Redux. Not everyone uses Redux. And even when you use Redux, some use cases still need weird code to be addressed e.g. side-effects and when local state is involved.

It's not more complicated than it needs to be.

I strongly disagree. Literally anyone I talked to who needs to do any of the stuff I listed above has told me otherwise.

navigationOptions are static because some of them need to be read statically. For instance ... DrawerNavigator

Sure, this is an addition to the API. For navigators which need static configuration to work, can enforce them by throwing a descriptive error. The primary goal of this is to make it easier to update the options dynamically. They can still be initialized statically, either with a static navigationOptions or by specifying the options when creating the navigator.

As an unrelated note, it probably makes sense to also render all screens in drawer navigator by default similar to TabNavigator so that the local state of each screen inside the drawer is preserved when switching between them (#2277). Users could still decide if they want to render the actual content or not using the new focus events.

Since HOCs are often applied using a decorator, how about a navigationOptions decorator.

The issue with HOCs is probably the least important one and can be easily fixed. I mentioned is because it's a side-effect the API. A different API using an HOC/decorator is still not ideal and easy to incorrectly use since it relies on the order in which HOC/decorators are applied.

@dantman
Copy link

dantman commented Feb 15, 2018

It probably seems easy for you because you use Redux. Not everyone uses Redux. And even when you use Redux, some use cases still need weird code to be addressed e.g. side-effects and when local state is involved.

I used React Navigation before I used Redux, I only use Redux for a subset of things (global state that was already global state before I used Redux), and I have yet to use Redux in any navigationOptions code.

Side topic: I don't even know of a way to access Redux from navigationOptions. One of the issues I haven't solved yet is how I'm going to localize the navigationOptions.title used by both my headers and drawer once I implement Redux backed localization storage. (And setOptions/NavigationOptions won't change this because data needs to be static for the drawer)

@dantman
Copy link

dantman commented Feb 16, 2018

I'll agree that the idea of being able to set some 100% active route only options from the instance would look nice, namely:

this.props.navigation.setOptions({
  headerRight: // ... my buttons
});

However I do believe that there are solutions to the issue of needing to use params.onSubmit issue other than having the instance completely override the options defined by navigationOptions. Like some sort of message passing or possibly the eventEmitter that has already been partially implemented in React Navigation.

Sure, this is an addition to the API. For navigators which need static configuration to work, can enforce them by throwing a descriptive error. The primary goal of this is to make it easier to update the options dynamically. They can still be initialized statically, either with a static navigationOptions or by specifying the options when creating the navigator.

I don't like how this can result in some code do being non-DRY.

i.e. If I want to make my title vary based on state but I still need a static title because my component is used in a drawer then the "adding dynamic setOptions but keeping static navigationOptions" way of handling this is to have to repeat the title.

// Static navigationOptions
navigationOptions = {
  title: 'My Profile',
};

// Instance
this.props.navigation.setOptions({
  title: this.state.isEditing ? 'Editing My Profile' : 'My Profile',
});

In this example I can just move isEditing to params.isEditing and go back to using params.isEditing ? 'Editing My Profile' : 'My Profile' in navigationOptions instead of using setOptions. However if I run into any other situation where a param is both needed statically by a component but I need it to communicate with the instance when it's not static, I have to duplicate the option because setOptions can't be accessed by the static navigator and navigationOptions doesn't have an options<->instance communication method.

If you need share state between component and header, for example to show something in the header based on the screen's local state, you need to update params everytime your component state changes, now you have 2 sources of truth.

params are not header state, they are route params/state. You should not need 2 sources of truth. If you have a piece of state that is relevant to the header, say counter, then you should not be mirroring counter from state.counter into params.counter, you should be using params.counter in both the header and your route.

As an unrelated note, it probably makes sense to also render all screens in drawer navigator by default similar to TabNavigator so that the local state of each screen inside the drawer is preserved when switching between them (#2277). Users could still decide if they want to render the actual content or not using the new focus events.

I was going to say I don't like the fact that this means all my drawer routes that do async initialization on mount would now have to register focus handlers and implement complex "Is this the first focus, or am I already initialized" handling just to avoid every inactive drawer route from slowing down app-startup and fetching data that will be out of data if the user ever switches. But I suppose that the fact the lazy option was removed from TabNavigator when the focus events were added means this was the plan anyways.


I see 3 possible options/solutions to our problems:

  1. Keep the static navigationOptions and add setOptions/NavigationOptions.
    I don't like this option because of the DRY issue.
  2. Make DrawerNavigator render all routes and make all navigationOptions dynamic. i.e. The drawerNavigator will use the dynamic title instead of needing the static navigationOptions title.
    I'm ok with this idea. However requires a pretty significant change to how React Navigation works and gets options in order for this to work.
  3. Keep navigationOptions static as it is. But implement better methods for navigationOptions and the instance to communicate with each other to solve the params.onSubmit issue.
    I'm also ok with this.

@satya164
Copy link
Member Author

satya164 commented Feb 16, 2018

the instance completely override the options defined by navigationOption

It won't override, it will shallow merge them, similar to .setState.

Like some sort of message passing

ex-navigation has it, I don't remember exactly, but it can't be the same event API. there's no way to manage a subscription inside navigationOptions. You would only be able to use it inside components.

duplicate the option

No reason you can't use a constant for those rare occasions.

It's not much different from initializing state in constructor and then updating it later with .setState.

params are not header state, they are route params/state.

params are not state. They are closer props. A component controls it's own state, however a component cannot initialize it's params, they must come from parent. This doesn't make it easy to use params as a local state container. I'm not even sure it should hold component state. Navigation state is similar to a URL to me, if it doesn't make sense to be in the URL, it doesn't make sense to be in params.

focus handlers and implement complex "Is this the first focus, or am I already initialized" handling

It's just a boolean check, nothing complex. And it can be abstracted out to an HOC to reduce boilerplate.

Anyway, we can probably introduce a bunch of new API and new patterns to solve the problem we encounter. But we are writing React apps and the more we make the library play with React, the easier we make it to learn and use.

@satya164
Copy link
Member Author

Came of with another API when talking to @brentvatne about it

function App() {
  return (
    <Screen
      options={{
        title: 'Hello'
      }}
      render={({ navigation }) => {
        return <Text>Hello world</Text>
      }}
    />
  );
}

With this, static navigationOptions are not needed anymore! Probably unlikely to happen, but just putting it out there 😂

@dantman
Copy link

dantman commented Feb 16, 2018

@satya164 How do you intend to communicate with the header? Do you just mean that the header component is inside the <Screen> component and that component is used by the routes? Or that the <Screen> component handles communication the way <NavitationOptions> would have?

It's an interesting idea, but keep in mind that sometimes Header is shared between routes instead of being present inside the Route's card. This is the case on iOS and possibly the case if you are trying to implement Material Design style shared element transitions on Android like you see in Google's apps (try Inbox for example).

We also still need something for DrawerNavigator and TabNavigator where options are used in shared elements (drawer sidebar and tab labels) outside of the route.

^_^ This kind of thing might be easier if there was some way React's Portals would let you create a Portal inside React instead of just a Portal to a dom node.

@satya164
Copy link
Member Author

Header can be anywhere, doesn't have to be inside Screen. The options object could be communicated with the header via React's context. Is similar to React's Portal pattern.

@dantman
Copy link

dantman commented Feb 17, 2018

Ok. Though I don't see how it's different from <NavigationOptions>, aside from the page content being in a render prop instead of being a sibling.

@satya164
Copy link
Member Author

This component can be rendered all the time, but the screen content doesn't have to be.

@brentvatne
Copy link
Member

issue that would be solved easily by this: react-navigation/react-navigation#940

@grabbou
Copy link

grabbou commented Feb 20, 2018

I second the API presented in previous comment. I've been partially prototyping this out in my free time recently, thanks to @ericvicenti pointing me towards that direction.

We had this idea of making it easy to just render a header component instead of title, right and other navigation options:

class HomeScreen extends React.Component({
  render() {
    return (
      <Screen header={<Header title="Welcome" />}>
        {...screenContent}
      </Screen>
    );
  }
});

This allows for easy customization of the header. If the header is not set on Screen, it will not be rendered at all. It also makes it easy for people to connect their state with the header which I guess has been one of the most common requests.

It's an interesting idea, but keep in mind that sometimes Header is shared between routes instead of being present inside the Route's card. This is the case on iOS and possibly the case if you are trying to implement Material Design style shared element transitions on Android like you see in Google's apps (try Inbox for example).

As @satya164 mentioned, this can be done with a similar to React's portal pattern - effectively, reusing the existing implementation of the header. However, I've been thinking to what degree we could achieve native-like look&feel of the header by having it separate on each screen and figuring out a new way for shared-element transitions.

@ericvicenti
Copy link
Contributor

Yeah, I think its a good idea to move the library in this direction. We need a nicer way of dynamic option configuration, and the convention here is rendering. But we should take an incremental approach, so the only thing that makes sense as the first step is navigation.setOptions . Maybe that would make for a good RFC?

@satya164, I'm curious how a component with static navigationOptions would behave if it also has a <NavigationOptions /> component inside of it. These options could conflict, no? I suppose we shouldn't support that usage at all.

Also I'd like to suggest that we have separate option config components and prop types for each navigator. That way we can implement cute things like this:

const HomeScreen = props =>
  <React.Fragment>
    <StackHeader
      title="Welcome"
      rightComponent={<MyButton />}
    />
    <HomeScreenContent />
    <TabBarItem icon={Icons.home} label="Home"/>
  <React.Fragment>

@dantman
Copy link

dantman commented Feb 21, 2018

@ericvicenti So the majority of navigator options that affect the UI (including the titles for headers, tabs, and drawers) get moved into render components and disappear from static navigationOptions?

I suppose we could even kill title itself. That would also solve my problem which is I have the same react-navigation/react-navigation#940 but for doing redux based i18n, except I want to be able to apply that to the navigationOptions.title used by the drawer in addition to the header, which hasn't been solved by most of the other suggestions that keep navigationOptions while adding dynamic options.

title probably isn't completely necessary if all the title handling is in render() where it's trivial to just use a title variable.

Just need to contemplate whether this may work with the iOS back title behaviour.

@satya164
Copy link
Member Author

satya164 commented Feb 21, 2018

I'm curious how a component with static navigationOptions would behave if it also has a component inside of it. These options could conflict, no?

My idea was to shallow merge the stuff from <NavigationOptions /> over static navigationOptions so that the config inside the component overrides the one defined statically. But not allowing makes sense if we wanna move away from the static configuration entirely.

except I want to be able to apply that to the navigationOptions.title used by the drawer in addition to the header, which hasn't been solved by most of the other suggestions that keep navigationOptions while adding dynamic options

This one can support all use cases without a static navigationOptions.

const HomeScreen = props =>
  <Screen
    header={<Header title="Welcome" />}
    render={() => {
      return <Text>Hello world</Text>
    }}
  />

@grabbou
Copy link

grabbou commented Feb 21, 2018

Yeah, with the above example, we can easily set navigationOptions based on header prop w/o rendering the screen itself ahead of time.

@dantman
Copy link

dantman commented Feb 21, 2018

My idea was to shallow merge the stuff from over static navigationOptions so that the config inside the component overrides the one defined statically. But not allowing makes sense if we wanna move away from the static configuration entirely.

Don't forget that navigationOptions config isn't exactly defined statically if it's a function. You're not merging setOptions with a single unchanging object. You're merging it with the result of a function that is re-executed whenever params change.

@ericvicenti
Copy link
Contributor

Don't forget that navigationOptions config isn't exactly defined statically if it's a function. You're not merging setOptions with a single unchanging object. You're merging it with the result of a function that is re-executed whenever params change.

Yeah, this is what has me confused. Seems like we will might momentarily show incorrect options on every props change because we get them from the static function and also in the options component

@dantman
Copy link

dantman commented Feb 21, 2018

We'll probably have to store the result of navigationOptions and the setOptions state separately for the instance so we can update the setOptions state and just merge the two into the final options object.

Or we could just dive straight into your StackHeader/TabBarItem/etc component API and provide some backwards compatibility by checking navigationOptions in the navigator when the dynamic option has not been set.

@dantman
Copy link

dantman commented Feb 28, 2018

@satya164 Is suggesting that this fixes the issue of context being inaccessible in screenProps, which leaves #5 open to removing screenProps.

Is this an accepted conclusion? If so then we are going to have to make sure that whatever solution we choose allows static properties such as title, drawerLabel, etc... access to context, because those properties can require things in context/screenProps as well (for example localizing drawer labels and the fallback title used for drawers, headers, back buttons, etc...).

@satya164
Copy link
Member Author

@dantman The solution we proposed in #24 (comment) should not require any static options since it allows to defer rendering of the screen to when necessary. It also works well with context and other patterns.

@dantman
Copy link

dantman commented Feb 28, 2018

@satya164 Sure, I'm just mentioning it because we have around 3 different proposals, some that still include static options, and don't seem to have consensus on exactly which one we want. And we're still talking about how to merge static navigationOptions with dynamic setOptions.

@grabbou
Copy link

grabbou commented Mar 5, 2018

I talked to @satya164 earlier today about getting a reference implementation in place and I am thinking I'll give it a go in my free time this week.

@lxcid
Copy link

lxcid commented Mar 30, 2018

I just add my 2 cents here against navigationOptions and for props & states:

  • As you use React, you often end up using HOCs, with navigationOptions and react-navigation, my HOCs get more complicate because I need to know where to hoist navigationOptions in my HOCs chain.
  • Sometimes, you need the navigation bar to participate with the screen state, there's no way to do this unless you pass the function through setParams, because of this workaround, we hit Component renders three times after reseting stack and calling setParams from componentWillMount react-navigation#3867. All I wanted was to pass function but I couldn't do that without setParams(), I can't hoist the state (with withState) and pass down either.

I love react-navigation and its way better than native navigation but I agree with @satya164 that this is pattern is more hacky…

EDITED:

To work around this issue, redux actually come to the rescue, by hoisting the state all the way to redux, I can use the navigation.dispatch as an escape patch to communicate with the application as a whole.

@KianooshSoleimani
Copy link

KianooshSoleimani commented Jun 27, 2018

its really simple you can make stateless component and connect it with redux like this :

   const view = ({ state, color }) => 
   {
   <View>
   <Text>{state.something}</Text>
   </View>
   }

   export default connect(
      (state) => ({
           data: state.data,
   }),
   null
   )(view);

and then use it in static options :

 static navigationOptions = ({ navigation }) => {
    return {
        tabBarIcon: ({ tintColor }) => <view color={tintColor}/>
    }
 };

@Yasser-G
Copy link

Yasser-G commented Dec 5, 2018

You Can call SetState or any other function inside your component from static navigationOptions using this simple lines

componentDidMount() {
        this.props.navigation.setParams({
            setState: this.setState.bind(this),
            myCustomFunction: myCustomFunction.bind(this),
        })
    }
static navigationOptions = ({ navigation }) => {
        const setState = navigation.getParam('setState', () => { })
        const myCustomFunction = navigation.getParam('myCustomFunction', () => { })
        return {
            headerRight: <TouchableOpacity onPress={() => setState({ buttonPressed: true })} />,
            headerLeft: <TouchableOpacity onPress={() => myCustomFunction()} />

        }
    }

Hope this may help someone.

@ravirajn22
Copy link

One year has completed since this RFC opened. Is any progress made on how to handle headerOptions using state directly?. Dynamically configuring NavigationOptions from screen state is one of the major bottleneck of this Library. @brentvatne @satya164 @ericvicenti @grabbou Pls have a fresh look into this.

@brentvatne
Copy link
Member

we are not working on this right now. we don't need a reminder of how much time has passed since this discussion started, it's here so that people can comment on it and provide their insights / ideas, and when someone wants to work on it, then it can be turned into a proper rfc and executed on.

@xXFracXx
Copy link

Any update?

@satya164
Copy link
Member Author

@tomasf10
Copy link

You Can call SetState or any other function inside your component from static navigationOptions using this simple lines

componentDidMount() {
        this.props.navigation.setParams({
            setState: this.setState.bind(this),
            myCustomFunction: myCustomFunction.bind(this),
        })
    }
static navigationOptions = ({ navigation }) => {
        const setState = navigation.getParam('setState', () => { })
        const myCustomFunction = navigation.getParam('myCustomFunction', () => { })
        return {
            headerRight: <TouchableOpacity onPress={() => setState({ buttonPressed: true })} />,
            headerLeft: <TouchableOpacity onPress={() => myCustomFunction()} />

        }
    }

Hope this may help someone.

Brilliant! Thanks! This help me to change the state of the modal visibility

@Yasser-G
Copy link

Yasser-G commented Nov 11, 2019

@toto104 You are Welcome, Use it wherever you like 😃 .

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

No branches or pull requests