ericvicenti / navigation-rfc

Better Navigation in React Native
441 stars 44 forks source link

Support for "replace" navigation action? #50

Open aksonov opened 8 years ago

aksonov commented 8 years ago

I implement 'replace' support with my own reducer (just changing state.children[state.index]), it works except I'm getting yellow warning setState can only update a mounted component - i believe i've broken built-in NavigationCard animation stuff (any way to disable it?). Any possible solution to avoid warning?

screen shot 2016-03-14 at 19 50 39
ericvicenti commented 8 years ago

cc @hedgerwang

hedgerwang commented 8 years ago

@aksonov : do you have a minimum repro?

perhaps you can reproduce this with the modified module.exports = NavigationCardStackExample?

aksonov commented 8 years ago

@hedgerwang It is difficult to provide minimum repo, because 'replace' action is not supported by NavigationCardStackExample. I was able to reproduce it in Example project for my react-native-router-flux component, please check 3.0alpha branch: https://github.com/aksonov/react-native-router-flux/tree/3.0alpha

I'm doing 'replace' action there within componentDidMount of Launch class (to Login screen).

hedgerwang commented 8 years ago

@aksonov :

Does replace mean replace current navigationState with another one? For that case, you could probably change the reducer at NavigationCardStackExample to this:


function createReducer(initialState) {
  return (currentState, action) => {
    switch (action.type) {
      case 'RootContainerInitialAction':
        return initialState;

      case 'push':
        return NavigationStateUtils.push(currentState, {key: action.key});

      case 'back':
      case 'pop':
        return currentState.index > 0 ?
          NavigationStateUtils.pop(currentState) :
          currentState;
      case 'replace':
          return action.state,
      default:
        return currentState;
    }
  };
}
aksonov commented 8 years ago

No, replace means to replace one scene to another scene (with unmounting of previous scene) - what old Navigator does

hedgerwang commented 8 years ago

I see.

I think you could start with adding replace to NavigationStateUtils then change the reducer to the following:

function createReducer(initialState) {
  return (currentState, action) => {
    switch (action.type) {
      case 'RootContainerInitialAction':
        return initialState;

      case 'push':
        return NavigationStateUtils.push(currentState, {key: action.key});

      case 'back':
      case 'pop':
        return currentState.index > 0 ?
          NavigationStateUtils.pop(currentState) :
          currentState;
      case 'replace':
          return NavigationStateUtils.replace(currentState, {key: action.key});
      default:
        return currentState;
    }
  };
}
aksonov commented 8 years ago

I did it already with own reducer. The problem in that warning. Probably because replaced component is unmounted before transition ends..

Pavel.

16 марта 2016 г., в 18:16, Hedger Wang notifications@github.com написал(а):

I see.

I think you could start with adding replace to NavigationStateUtils then change the reducer to the following:

function createReducer(initialState) { return (currentState, action) => { switch (action.type) { case 'RootContainerInitialAction': return initialState;

  case 'push':
    return NavigationStateUtils.push(currentState, {key: action.key});

  case 'back':
  case 'pop':
    return currentState.index > 0 ?
      NavigationStateUtils.pop(currentState) :
      currentState;
  case 'replace':
      return NavigationStateUtils.replace(currentState, {key: action.key});
  default:
    return currentState;
}

}; } — You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

hedgerwang commented 8 years ago

Do you want the replaces navigation state to go way with animation or just be removed immediately?

What kind of warning that you were getting? Could you please share a screenshot for that? Thanks.

aksonov commented 8 years ago

Screenshot is shown at first message - i was not able to see where is that setState call. I could live without animation, but community of my component also want possibility of animation... The problem happens only if i do replace within componentDidMount.

chandu0101 commented 8 years ago

any update on this ?

just showing my interest in this issue/feature :)

aksonov commented 8 years ago

After upgrading to 0.24rc, replace action stops working at all, @hedgerwang, @ericvicenti Is there any docs how to migrate from 0.22 to 0.24rc (or 0.23)? I'm just replacing current element in the state with new one within reducer. It worked well within 0.22, but not for 0.24rc (0.23)

ericvicenti commented 8 years ago

If the new child state has a new key, it should get replaced. Otherwise it should get re-rendered. I'll look into it this afternoon. There are a bunch of reports on this, so it looks like something is seriously broken 😢

zackify commented 8 years ago

@ericvicenti yep, broken for me too.

mmazzarolo commented 8 years ago

Same issue here even on 0.25.1... any progress on this?

aksonov commented 8 years ago

@mmazzarolo Seems 0.26-rc works fine.

mmazzarolo commented 8 years ago

@aksonov thanks man! Edit: nope, it is still not working for me.
Replacing the entire stack doesn't re-render the app. We should probably wait for this.