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

goBack() additional functionality #905

Closed
wants to merge 6 commits into from
Closed

goBack() additional functionality #905

wants to merge 6 commits into from

Conversation

harakhovich
Copy link

@harakhovich harakhovich commented Apr 4, 2017

I and many other people meet lack of base functionality provided by RN Navigator such as .popToTop and .popN().
This pull request add such functionality by modifying input params for .goBack()

Now it accepts such objects:
.goBack({ key: 'your-route-key' })
.goBack({ numberOfPages: 3 }) - goes back numberOfPages screens
.goBack({ routeName: 'YourRouteName' }) - goes back to the screen in stack with such routeName.

Adding these stuff other issued popped -- when moving back few screens previous screens are flashing. I added work around, but would like to hear a solid solution to fix that before merging PR. This requires tests to be fixed.

Now I use these branch and these functions on my own production project and it works perfectly!

@Kerumen
Copy link

Kerumen commented Apr 4, 2017

What .goBack({ key: 'your-route-key' }) does? I don't understand this param.

Maybe rename numberOfPages to numberOfScreens because screen is conventional name in react-navigation.

@alimek
Copy link

alimek commented Apr 4, 2017

@Kerumen I would say that .goBack({ key: 'key }) will do the same what .back({ key: 'key' }) does.

Waiting for this to be merged 👍

@Kerumen
Copy link

Kerumen commented Apr 4, 2017

@alimek And what .back({ key: 'key' }) does? Sorry but I really don't get it.

@ericvicenti
Copy link
Contributor

It makes sense to add a 'goToTop' action to satisfy the use case you mentioned.

I really think the key is enough for most back actions. When a screen fires an action to go back two 'pages', how does the screen know how far the destination screen is? If you add a new screen in the middle of your flow, your final screen will only go back a certain amount, and won't go back all the way. I think the current API is nice because it encourages you to decouple your screens. If you have a more complicated navigation behavior, it may make sense to customize the router and add your own navigation actions.

The issue with ScenesReducer is interesting, and I think your approach makes sense, even if the code is super confusing. Basically what you're doing is to only render one stale scene (the one that was on top), rather than all of the stale scenes. Can you submit this in a separate PR, and make the variable names a bit more specific?

@ericvicenti ericvicenti closed this Apr 5, 2017
@ghost
Copy link

ghost commented Apr 5, 2017

@Kerumen .goBack({ key: 'your-route-key' }) it does what it did before with .goBack(key). It's the same behavior, just another syntax.

@ghost
Copy link

ghost commented Apr 5, 2017

@ericvicenti this PR came from needs of my project. We were migrating from RN Navigator to this one. We had lots of popN and popToTop functionality. There were no such functions in react-navigation so that's why I added it by myself.

re: ScenesReducer. Sure I can and will, but current solution looks hacky :) I would like to go deeper and maybe change it to more robust and cleaner solution.

@pvinis
Copy link

pvinis commented May 16, 2017

@ericvicenti
"I think the current API is nice because it encourages you to decouple your screens."

how exactly is the current api help with decoupling, when we need to pass the key of the screen to go back from?

are you serious? you and @grabbou, and the rest of the main committers/maintainers are all handing this library like its your personal thing, and you are being proud that its featured in many places online, and your just resting on your laurels. the last like 6 months, this project had no real change. there are so many issues of missing features and so many prs that should be merged, and the response to most of them is "this is not needed." (no explanation), or "i like it the way it is now.", or whatever else.
if you need more help/people to maintain and really review issues and merge prs, ask for more people. many of us here, including me, would love to help make this library actually usable in production apps.
if you just dont wanna change it because it "works for you", then rename it to whatever-navigation, and move it away from react-community, and let us do a better job at making a community library.
react-navigation is the most promising library, for over half a year now. and it has moved so little, its the worst managed navigation library of them all. i want to use it. but i have been waiting for things to get merged from others, waiting for things of mine to get merged, and still i can use it cleanly. ask anyone with any app that is more complex than your example app, how he uses this library. there will be literally zero people that use it without some kind of wrapper/helper/manager/workaround. zero people. this should not be the case.

please please, allow people to help you, or move it away. thank you.

@ericvicenti
Copy link
Contributor

ericvicenti commented May 16, 2017

Sorry about the troubles you're experiencing with the library. This is a large project and isn't very easy to organize. Everybody needs different features and the library is designed to allow you to override and customize things as you need. And if you really do need to fork it for your organization, go right ahead!

I'm happy to review PRs but I think we need to be careful about adding a bazillion features that will make the library even harder to maintain than it is today. cc @skevy who is spending more time on it and is also interested in keeping it simple and maintainable.

how exactly is the current api help with decoupling, when we need to pass the key of the screen to go back from?

Say you want to build a portable modal that can be dropped into many apps. The modal has a 'close' button, and when you press it, you want to go back. If you dispatch(NavigationAction.goBack(this.props.navigation.state.key) then the screen is explicitly saying that you wants to close itself. If goBack accepts a destination key or routeName, then the modal will only work in one app, because you've introduced coupling to that app's routes!

For this issue, I think we should support dispatch(NavigationActions.navigate({ key: myKey }). If the key exists in the stack, we will navigate to it (even if it is 'back'). This feature is being tracked in #135. If you'd like to contribute, a pull request for it would be appreciated!

If you have specific suggestions or requests regarding community management, feel free to open up another issue to discuss it! We're absolutely interested in doing better.

@pvinis
Copy link

pvinis commented May 16, 2017

i have forked to try and fix a couple of things, but forking is not the solution always.

about the modal: the modal would just call goBack(), with no arguments. it should just go back, no need to specify any key. and you can put this modal anywhere you want, and it will work.
the case where specifying a key, or better, a routeName is useful, is when you dont want the regular goBack action. and in these cases, coupling is unavoidable, because coupling is what you want. i have my regular goBack, and it works with drop it things. and i sometimes want to have a funky goBack to go back to screen Bla, because i clicked a button that should take me there. so i should be able to specify goBack('Bla'), and be done with that.

the navigate idea is also bad. by definition, a stack goes two ways, it pushes, and it pops. if i want to add a screen, it doesnt matter if its there already in the stack somewhere. i want to push, so i will do a navigate('Blu'), and i will go to that screen that way. if i want to go backwards, then i will do a goBack('Ble') and it will pop everything up to this screen, and take me there. this is what navigation is. if you want to add stuff, or pop stuff, i should decide, and it should be clear. navigate for pushing, goBack for popping, and reset for resetting, should be all we need. no more, no less.

this is also a thing im sure can be better. consistency of this project. things are all over the place. you have indexes, but they are not visible in the navigationOptions. you have keys to go back, but mostly the keys are autogenerated. you have three different ways to call goBack, and the docs say that goBack() and goBack(null) are different. you have reset but to actually reset i need to specify index: 0. all these ideas make no sense, and it feels like 10 people made some random decisions, some better than others, but then you just took all of them and noone thought of an integration plan for all. its just a bunch of random things made by different people that just happens to be able to run.

@pvinis pvinis mentioned this pull request May 16, 2017
5 tasks
@ericvicenti
Copy link
Contributor

the navigate idea is also bad.

Jeez, if you say so. Push and pop work great for a simple stack, but we're trying to find a set of actions that can navigate a tree of routes. Thats why navigate and goBack are different.

You're probably right that things could be more consistent. Do you have any actionable ideas on how it can be improved?

@pvinis
Copy link

pvinis commented May 16, 2017

i made this new issue so we can talk about some stuff there.

and i dont mean it in a bad way, dont get me wrong, but in my experience, when you try to have one method for those functionalities, it either doesnt work in the end, or its just a huge monster that could be split to smaller parts. push and pop work fine for a tree of routes, because in the end, its is a simple stack. the screens are a big tree, but the screens the app followed at any point, is just a simple stack of screens (tree nodes). its just pointing to them.

@jameskhamil
Copy link

jameskhamil commented Jun 13, 2017

@pvinis - I wasted a bunch of time trying to integrate this navigator into a large project and am not surprised to see so many PRs and fixes going ignored or being glibly rejected. Why is anybody using and trying to contribute to this as opposed to aksonov/react-native-router-flux? I don't understand, is every package just inadequate right now? How is anyone shipping RN projects with complex navigation, let alone something that needs to use basic back functionality?

There should be a big "UNSUITABLE FOR PRODUCTION PROJECTS" on the homepage instead of a nicely designed website.

@huyansheng3
Copy link

huyansheng3 commented Jun 15, 2017

@ericvicenti when will the pr be published as a release version?

@pvinis
Copy link

pvinis commented Jun 27, 2017

@jameskhamil - the reason i moved to react-navigation from react-native-router-flux, was that i liked the fact that each screen was customized in the screen's component. so basically before i had all the navigation options in the big nav file with all the screens, and now i have them on each screen component. to me, it makes sense.
the second thing i liked was the native-like transition between screens.
i am sure most production react native apps are just ugly in the source. everyone tries to make it nicer, me included, but it takes time.
specifically for this pr, and a couple others that i have made, yea. lets me. shall we have a race on what will come first: these prs in a beta or january 2018?

@guoliang1206
Copy link

@ALL so at now ,how do we realize popToRoot likes iOS platform: popToRootViewController ? Without the splash of the middle screen?

@NSShentu
Copy link

+1 !!!!!
@ALL so at now ,how do we realize popToRoot likes iOS platform: popToRootViewController ? Without the splash of the middle screen?

@NSShentu
Copy link

NSShentu commented Jul 11, 2017

@ericvicenti as a developer, i really really want pay not so much attention to how i deal with the routers. i hope i can care more about my own works, rather how i get the key, pass the key, then i can jump the key screen. You think routerName is same to key. But i'd like to use goBack(LogPage) rather than goBack({key: 'id-14764567767678-11'}), right? To me, react navigation is a tool to deal with router. As a tool, its API should adapt to more situation directly.
@ericvicenti help!!!!======>>> so at now ,how do we realize popToRoot likes iOS platform: popToRootViewController ? Without the splash of the middle screen?

@ollyde
Copy link

ollyde commented Jul 19, 2017

Gawd this is really really awful. I've wasted countless hours on this junk navigation, keep it simple guys..
Anything I try doesn't work, even using the screen's id on goBack didn't work.

dispatch(NavigationAction.goBack(Store.getState().screenKeys.startScreenKey));
Nope

goBack({routeName: 'ScreenStart'});
Nope

goBack({key: this.props.screenIdToGoBackTo});
Nope

What am I to do? The documentation is useless and usually outdated.

@guoliang1206
Copy link

guoliang1206 commented Jul 20, 2017

I don't know that why they ignored this issue.

navigation.goBack(); is useless to go back N pages.

@ranran2
Copy link

ranran2 commented Jul 21, 2017

@voidstarfire the way to get the key of screen(named ScreenA) is from the state like this this.props.navigation.state.key(named ScreenAKey), you can pass this key to next Screen(named ScreenB) by navigate will a param, and then you can pass it again to next screen; when you want to goback ,you just go back like this ' this.props.navigation.goBack(ScreenAKey)' . Thougn it's inconvenient.

@quadsurf
Copy link

quadsurf commented Jul 30, 2017

@ranran2 lol, that doesn't even work... and the first thing you're probably going to say, is that I'm not doing it right (the proper context of this.proper.navigation is lost whenever it's accessed from child components not registered as screens)

this "popToTop" kind of feature is not an edge case... the most common use case for needing this "popToTop" feature, is when a user logs out of the app... in fact, "popToTop" is not even a good solution for this, especially if you need your root component/index/screen to be re-rendered... NavigationActions.reset is the only thing that comes close, but unfortunately since the routeName is scoped to the current sub-stack and not the root/parent stack, 'reset' cannot solve this common use case either

a goBack() method (one that actually works globally across the app) has it's proper use cases, but logging a user out is not one of them... for a proper logout function, 'this.props.navigation.goBack(screenKey)' is not the correct way to do this since it navigates/pops you to an already mounted component as opposed to the expected/intended "reset then push" pattern

@ollyde
Copy link

ollyde commented Jul 31, 2017

The reset functionality also comes with a nasty animation of 'all the screens'! woot...

The custom transitions are complicated as hell to work out.

@kitsune7
Copy link

Aw jeez, this is all just a mess... (-_-)

@coolguy001tv
Copy link

so the goBack functionality is still NOT merged ?

@ranran2
Copy link

ranran2 commented Aug 24, 2017

@quadsurf you can pass the navigation to child component. i use this in my project and it works.lol

@rtman
Copy link

rtman commented Sep 8, 2017

@ericvicenti @satya164 @skevy

Are you guys still maintaining this library? It seems like there's some major work to be done and people willing to do it, but they need to be given access. I think there are a lot of people building production apps around this library and it would be great if fixes could be applied for the bugs found. Go back is especially hard to deal with.

@musicode
Copy link

Any updates?

@hoscarcito
Copy link

Is it possible to work on the popToTop functionality mentioned here??
It's really needed, we don't wanna loose the history with a reset and there's no other option.
The popN solution is good (.goBack({ numberOfPages: 3 })) but it lacks the ability to pop to top if you don't provide the route length.
Thanks in advance

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.