Skip to content
This repository was archived by the owner on Feb 25, 2020. It is now read-only.

refactor: store more info about the transitions in the state#64

Closed
satya164 wants to merge 1 commit intomasterfrom
@satya164/stack-router
Closed

refactor: store more info about the transitions in the state#64
satya164 wants to merge 1 commit intomasterfrom
@satya164/stack-router

Conversation

@satya164
Copy link
Member

@satya164 satya164 commented May 29, 2019

I'm working on reimplementing stacks and storing this info in the routes state makes things much simpler and allows me to get rid of something like the transitioner.

@satya164 satya164 force-pushed the @satya164/stack-router branch 3 times, most recently from d8f0324 to e343e8e Compare May 29, 2019 13:50
@satya164 satya164 force-pushed the @satya164/stack-router branch from e343e8e to 2ec8cbc Compare May 30, 2019 00:12
@ericvicenti
Copy link
Contributor

Hey, sorry I didn't see this earlier! I wish we had discussed it before you spent time on this direction, because I think it is not such a good idea to keep transition info in the navigation state. The only reason isTransitioning is in the state is because we need to notify the event system when a transition is happening.

I really think the transition state should be handled by the navigator component, and not by navigation core. There are lots of edge cases that may crop up.. like, what if a navigator has a type of transition other than pushing or popping?

I see the isTransitioning stuff has been removed from Tab Router, which would break the "didFocus/didBlur" events in an animated tab navigator.

I actually think we should remove isTransitioning from the navigation state, and provide another API for transition completion instead (check the core/stack/native "transition-completion" branch to see how that would work)

@satya164
Copy link
Member Author

satya164 commented Jun 6, 2019

Thanks for checking. I think it's okay coz this change was not a lot of work and I needed it to continue with my work regarding stack, so I couldn't wait. I can easily change it when we arrive at an alternative API.

The only reason isTransitioning is in the state is because we need to notify the event system when a transition is happening.

My use case is not for the event system. I'm rewriting stack navigator with reanimated: react-navigation/stack#131

The reasons I added this to router are:

  • With reanimated, it's important to make sure only one route animates at a time to avoid some issues, so I need to keep track of which routes are transitioning: https://github.com/react-navigation/stack/blob/eb8aa9a34c946ea16839f9a11d87510d6d25939d/src/views/Stack/StackView.tsx#L45
  • There is immediate: boolean to control if pushing/popping something should cause an animation or not, there's also replace which won't cause an animation. I need access to this info somehow. Since the view only receives the list of routes from the router, it’s not possible for it to know if a screen should animate or not when being added/being removed.

I see the isTransitioning stuff has been removed from Tab Router, which would break the "didFocus/didBlur" events in an animated tab navigator.

The tab navigator never used the isTransitioning property. So I think it shouldn't break anything. I think only stack navigator needed it.

I actually think we should remove isTransitioning from the navigation state, and provide another API for transition completion instead

I don't have any opinions about it, and it does seem better to remove this from the router. But the main issue is how can I access this info about which routes will animate from my component? I did take a look at the branch, but I think that's mostly related to the event system only.

@satya164
Copy link
Member Author

satya164 commented Jun 7, 2019

Closing as we agreed on an alternative way.

@satya164 satya164 closed this Jun 7, 2019
@satya164 satya164 deleted the @satya164/stack-router branch June 7, 2019 23:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments