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

Best pattern for a 'Save' button in the header #145

Closed
despreston opened this issue Feb 1, 2017 · 113 comments
Closed

Best pattern for a 'Save' button in the header #145

despreston opened this issue Feb 1, 2017 · 113 comments

Comments

@despreston
Copy link

Often times if you are editing or creating something, one of the header buttons will be 'Save'. I don't see how this is feasible with this library since there is no access to the state or props of the current screen component. The only (ugly) solution would be to keep the data to be saved outside of the component. Anyone have any suggestions how this pattern can be achieved with this library?

@despreston
Copy link
Author

despreston commented Feb 1, 2017

On a somewhat related note, Ive seen references to a Slack channel. That would be a better place for this discussion but I'm unsure of how to get access to that channel.

@dmhood
Copy link

dmhood commented Feb 1, 2017

There is some discussion on the reactiflux discord server (https://www.reactiflux.com/), specifically in the rn-navigation channel, although I don't think its specifically for this library.

@chuckhacker
Copy link

Related: #149

@Schniz
Copy link

Schniz commented Feb 2, 2017

the only solution I can think of is using setParams to set the callback/state in the controller and using state.params in the header

@despreston
Copy link
Author

Thanks @Schniz . Thats the pattern I was thinking as well and the path that I ultimately took. Unfortunately I ran into what appears to be a bug related to calling setParams. Ive opened a separate issue for that bug. All in all, this pattern seems a bit hazy and not entirely 'safe' unless you explicitly check for the existence of all state.params.

@Schniz
Copy link

Schniz commented Feb 2, 2017

Yeah. I think that state.params should default to an empty object.

@Schniz
Copy link

Schniz commented Feb 2, 2017

However, I really dig using ramda's pathOr for these kind of problems.

import { pathOr } from 'ramda';
const noop = () => {};
// ...
const somethingToDoFn = pathOr(noop, ['params', 'somethingToDoFn'], navigator);
somethingToDoFn('data', 'to', 'pass');

@dmhood
Copy link

dmhood commented Feb 2, 2017

Isn't the proper way to do this:

componentDidMount () {
    const isDisabled = <someLogicUsingLocalState>;
    const params = {
            right: (
                <Button
                    onPress={() =>!isDisabled ? this.onSave() : null}
                >
                    <Text style={[styles.rightButtonTextStyle, isDisabled ? styles.disabledStyle : null]}>
                        Save
                    </Text>
                </Button>
            ),
        };
    this.props.navigation.setParams(params);
 }

However, #160 is preventing this from working well.

@dmhood
Copy link

dmhood commented Feb 4, 2017

Thought I would post my main navigation options too for completness sake:

function getCurrentParams (state) {
    if (state.routes) {
        return getCurrentParams(state.routes[state.index]);
    }
    return state.params || {};
}

...some setup...

// main stack navigator navigationOptions
navigationOptions: {
    header: ({state}) => {
        // get the "deepest" current params.
        const currentParams = getCurrentParams(state);

        const left = currentParams.left;
        const right = currentParams.right;
        const style = currentParams.style;
        const tintColor = currentParams.tintColor;
        return { left, right, style, tintColor };
    }
 }

This feels kinda hacky to me, but I don't see another way to set dynamic header properties other than the use of setParams (and again, #160 makes this a poor solution at the moment).

@grabbou
Copy link

grabbou commented Feb 9, 2017

Given that #160 is already fixed, do you think @dmhood your solution can become more standardised? Do you think it deserves a doc How to just for the sake of avoiding these questions in the future?

@dmhood
Copy link

dmhood commented Feb 10, 2017

If people find that useful then I'll make a pr here in the next couple of days, and we can standardize/critique it there.

@prodz18
Copy link

prodz18 commented Feb 12, 2017

@dmhood solution works great. I was looking to show a modal view when pressing one of the header buttons (left or right). Thanks!

@dmhood
Copy link

dmhood commented Feb 14, 2017

FWIW using setParams in componentDidMount is apparently discouraged, (see #296). I've mostly gone to pulling most of the "initial state" logic to set header buttons out into static/helper functions, which isn't great but should be doable unless your component uses something external like react-context.

@raymundtusk7
Copy link

I'm not sure I understood @dmhood's solution. How do I call the saveDetails() function? This code throws error, by the way: "_this2.saveDetails is not a function". Help would be appreciated. Thanks!

class MyScreen extends React.Component {
    static navigationOptions = {
        header: {
            right: <Button title={"Save"} onPress={() => this.saveDetails()} />
        }
    };

    saveDetails() {
        alert('Save Details');
    }

    render() {
        return (
            <View />
        );
    }
}

@dmhood
Copy link

dmhood commented Feb 21, 2017

@raymundtusk7 you can't call this.saveDetails() from a static context since the actual saveDetails method is a class method. You would need to make saveDetails static or just some helper function outside of the class.

@Schniz
Copy link

Schniz commented Feb 21, 2017

Maybe all the functions (when declaring navigationOptions as a function for instance) should receive the actual component as an argument

@raymundtusk7
Copy link

Can't seem to wrap my head around this. If I make the saveDetails static, would it have access to the screen's state object?

Another scenario:
Let's say I have a login screen with 2 text inputs and a login button at the header's right component. How would it send the details to the REST API via fetch?

@stevehollaar
Copy link

Try setting your component instance's handleSave function as a navigation state parameter, after the component has mounted. Something like:

class MyScreen extends React.Component {
    static navigationOptions = {
        header: ({ state }) => ({
            right: <Button title={"Save"} onPress={state.params.handleSave} />
        })
    };

    saveDetails() {
        alert('Save Details');
    }

    componentDidMount() {
      this.props.navigation.setParams({ handleSave: this.saveDetails });
    }

    render() {
        return (
            <View />
        );
    }
}

@raymundtusk7
Copy link

@stevehollaar: Awesome! This worked for me though:

<Button title={"Save"} onPress={() => {state.params.handleSave()}} />

But you pointed me to the right direction. Thanks a lot! 💯

@stief510
Copy link

Is there a way to dynamically enable or disable it? Meaning, if the user changes input the Save button would be enabled

@grabbou
Copy link

grabbou commented Feb 23, 2017

@stief510 yes, please see NavigationPlayground - SimpleStack example, go to Profile. There's Edit right button. Same methods will work for you to handle input changes.

@stief510
Copy link

@grabbou But is there a way to dynamically do so from the component itself? I know you can change it manually, like if the user presses something. But what if the we automatically detect that the user filled out the whole form? Can I automatically enable the button somehow?

@satya164
Copy link
Member

I'm playing with something which might solve your issues https://github.com/satya164/react-navigation-addons

This is just an exploration. Feedback welcome. :D

@stief510
Copy link

@satya164 Looks exactly like what I need. I'll check it out

@jezravina91
Copy link

@fqborges I used your solution and works great. I just have a quick question. There's a bit of delay before the button is rendered on screen, so the default button of react-navigation is rendered first and is then replaced with the custom button on your solution. Is there anyway that the custom button is rendered first or before the component mounts?

@Jacse
Copy link

Jacse commented Nov 7, 2017

@Nitro2003 That's because the header params are only set after component mount. Take a look at the docs here, they explain how to avoid it.

@Jacse
Copy link

Jacse commented Nov 7, 2017

@a88zach alternatively you could call setParams at the same time you call setState. E.g.:

onTextChange = (text) => {
    this.setState({
        theText: text,
    });
    this.props.navigation.setParams({
        hasErrors: this.hasErrors(),
    });
}

Just note that setState is asynchronous so if hasErrors depends on reading state you should call it in a callback or supply it with the new state as a param.

@kkusanagi
Copy link

non of the above work now. WHY?

the best answer code as below.

componentDidMount() { this.props.navigation.setParams({ handleSave: this._saveDetails }); }

this will do an infinity render. so can't work.

the BEST of THE BEST solution is create your own header and customise yourself.
use navigation function like

this.props.navigation.goBack()
this.props.navigation.navigate('HOME')

use react navigation as a tool to change screen only. that it.

@Jacse
Copy link

Jacse commented Nov 8, 2017

@kkusanagi It wont infinitely render. Setting navigation params does not trigger a component update. The code in the docs works and has been tested numerous times.

@kkusanagi
Copy link

@Jacse thanks for the remind. I miss checking the link added above.
yes, from the documentation, it sound work. just a bit strange of why need to change
_functionname (){} TO _functionname = () =>{}

https://reactnavigation.org/docs/intro/headers#Header-interaction-with-screen-component

Anyway, I also give up on using react navigation header. Cause in my case, I need to fully handle header like changing background and animated hide the header.

@wellyshen
Copy link

wellyshen commented Nov 12, 2017

Hi Guys, I must say set action through the setParams in componentDidMount will make Stacknavigator transition (e.g. modal pop-up) slow. Does anyone aware of that? I think this library should provide an official way or performance improvement to handle this issue.

@nonameolsson
Copy link

@kkusanagi That is because the binding. With fat arrow functions you get the correct binding in this case.

@wellyshen
Copy link

wellyshen commented Nov 13, 2017

@nonameolsson I used fat arrow functions for it as this document. And it causes the slow transition.

@rajat1saxena
Copy link

rajat1saxena commented Dec 20, 2017

I've to check if the state.params is not undefined unlike other examples where people are directly setting onPress to state.params.handlePost. I am getting state.params is undefined error otherwise. What am I doing wrong?

static navigationOptions = ({navigation}) => {
    let postHandleCallback = null;
    if (navigation.state.params && navigation.state.params.hasOwnProperty('handlePost')) {
      postHandleCallback = navigation.state.params.handlePost;
    } else {
      postHandleCallback = () => {};
    }
    
    return {
      title: 'New Post',
      headerRight: <TouchableOpacity
                      style={{marginRight: 16}}
                      onPress={postHandleCallback}>
                    <MaterialIcons name='check' size={iconSize()}/>
                   </TouchableOpacity>
    }
  }

@betiol
Copy link

betiol commented Jan 13, 2018

My Solution:

static navigationOptions = ({ navigation }) => ({
  headerRight: (
      <TouchableOpacity
        onPress={navigation.state.params.openModal}
        style={{ paddingRight: 5 }}>
         <Icon name={"add-circle"} size={35} color={"#fff"} />
      </TouchableOpacity>
  )
});

componentDidMount() {
    this.props.navigation.setParams({
      openModal: this.modal.bind(this)
    });
 }

modal() {
    this.setModalVisible(true);
}

@sumitk
Copy link

sumitk commented Jan 24, 2018

This is what worked for me:

`componentDidMount() {
this.props.navigation.setParams({
openModal: this.modal.bind(this)
});
}

modal() {
console.log(this.state.modalVisible);
this.setState({modalVisible:true});
}

static navigationOptions = ({ navigation }) => ({
headerRight: (
<Button onPress={() => {navigation.state.params.openModal()}} >
)
});`

@corasan
Copy link

corasan commented Apr 12, 2018

None of these works for me

static navigationOptions = ({ navigation }) => ({
    headerRight: <Button onPress={() => navigation.state.params.updatedUser} />,
})

componentDidMount() {
    this.props.navigation.setParams({ updateUser: this.updateUser })
}

updateUser = () => {
    console.log('user updated')
}

@mgtitimoli
Copy link

@corasan you are passing

() => navigation.state.params.updatedUser

While based on your code, you should be passing

navigation.state.params.updateUser

to Button.onPress

@corasan
Copy link

corasan commented Apr 13, 2018

@mgtitimoli if I do that I immediately get an error, before the view even renders

Edit: Fixed it, thank you! You actually pointed me in the right direction to find out what I was doing wrong exactly

@tectonicpie
Copy link

@corasan What was the correct fix?

@SoldierCorp
Copy link

@betiol How can I do that if instead the TouchableOpacity I am using a custom header passing an array with the names of the right buttons to render them in the component?

@sydneywu
Copy link

@ rajat1saxena i also received this.props.navigation.state.params is undefined error. I realized that it is because i have the stack navigator inside the tab navigator. So the state i am looking at is the top navigator state. Try see if there is a routes array, this.props.navigation.state.routes. Your params may be in there.

@benjaminreid
Copy link

benjaminreid commented May 16, 2018

@a88zach’s solution worked best for me to keep state and params in "sync", #145 (comment).

@silasrm
Copy link

silasrm commented May 22, 2018

Hi,

I'm has a drawer (StackNavigator) with header, inside the root navigation (SwitchNavigator) without header. Like this:

Root (SwitchNavigator)
---- Drawer (StackNavigator)
--------Navigation

How I'm change this header/navigation info in drawer (StackNavigator) navigationOption?

@armsteadj1
Copy link

Similar to @benjaminreid -- I ended up taking @a88zach's approach and created a HOC to make it a little less work and ugly.

https://www.npmjs.com/package/react-navigation-underscore

@jrhee17
Copy link

jrhee17 commented Jun 11, 2018

Very concerning a feature like this requires so much workaround

In my case, pressing the header button before componentDidMount also caused an error.

Had to resort to something like this

static navigationOptions = ({ navigation }) => {
    const {
      params = {
        saveItem: () => {},
      },
    } = navigation.state;
    return {
      title: 'Foo Bar',
      tabBarVisible: false,
      headerRight: <Button title="Save" onPress={() => params.saveItem()} />,
    };
  };

@brentvatne
Copy link
Member

@jrhee17 - I don't know what you're trying to get at by saying it's "very concerning" - please use objective language when discussing issues so it's easier to be actionable.

I agree this API isn't ideal, we should use an event emitter, see react-navigation/rfcs#48

@react-navigation react-navigation locked and limited conversation to collaborators Jun 11, 2018
@brentvatne
Copy link
Member

brentvatne commented Jun 11, 2018

I should also mention in the above example you don't actually need to provide a noop function, you could just do onPress={params.saveItem} - the convention for onPress handlers is to do nothing if the callback is null. If you wanted to provide a fallback fn, you could do navigation.getParam('saveItem', () => console.log('no item!')) where the second arg is the default value for the param

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests