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

apply params state change during navigate in TabRouter #134

Closed
wants to merge 1 commit into from

Conversation

danj3
Copy link

@danj3 danj3 commented Feb 1, 2017

Related to Issue #80

params is not merged into state during the Navigation dispatch, so is unavailable to the Screen. I make no claim of understanding this code base, however, running through the nav transitions in the debugger led me to conclude that this is simply not handled. If it is handled then there is a documentation hole explaining how it is supposed to work, though I don't know what that would be.

Test

Object transition during action with params, attached png

navigate_params

Copy link
Contributor

@ericvicenti ericvicenti left a comment

Choose a reason for hiding this comment

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

Hi, can you please write a few tests that demonstrate this fix?

You can use the existing src/routers/__tests__/TabRouter-test.js

@@ -129,6 +129,10 @@ export default (
}
if (activeTabIndex !== state.index) {
// console.log(`${order.join('-')}: Normal navigation`, {lastIndex: state.index, newIndex: activeTabIndex});
if ( action.params ) {

This comment was marked as abuse.

@@ -129,6 +129,10 @@ export default (
}
if (activeTabIndex !== state.index) {
// console.log(`${order.join('-')}: Normal navigation`, {lastIndex: state.index, newIndex: activeTabIndex});
if ( action.params ) {
state = {...state, routes: [...state.routes]};
state.routes[activeTabIndex] = {...state.routes[activeTabIndex], params: action.params};

This comment was marked as abuse.

@satya164
Copy link
Member

satya164 commented Feb 7, 2017

Any updates on this @danj3 ?

@danj3
Copy link
Author

danj3 commented Feb 7, 2017

I hope to attend to the changes shortly.

@raduflp
Copy link

raduflp commented Mar 11, 2017

any progress on this?

@ericvicenti
Copy link
Contributor

This is a pretty simple and high-pri fix. @danj3, any updates? If anybody else wants to address the feedback and submit a PR for this, we can land it sooner rather than later

@mgtitimoli
Copy link

mgtitimoli commented Apr 9, 2017

I've just found this one 😑 ...

I've also resolved #80 on this PR. Hope it lands to master soon.

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

5 participants