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

Explicit attaching and detaching viewControllers in store #4947

Merged
merged 5 commits into from
Apr 8, 2019

Conversation

yogevbd
Copy link
Collaborator

@yogevbd yogevbd commented Apr 4, 2019

Register component when viewController is created and release it on dealloc.
This PR allow's more explicit behaviour of attaching and detaching components.
This also solves crashes on multiple setRoot and other edge cases

@guyca guyca merged commit 2830059 into master Apr 8, 2019
@guyca guyca deleted the storeUsageRefactor branch April 8, 2019 06:22
modavi added a commit to modavi/react-native-navigation that referenced this pull request Apr 8, 2019
@Collin3
Copy link

Collin3 commented Apr 18, 2019

I think there is a regression from these changes related to the topbar buttons. I upgraded from version v2.16.0 to v2.17.0 and now my top bar buttons are not showing up. I have a right button component defined and set with setDefaultOptions, but that doesn't seem to be working anymore. To get around it I have to define static options or do a mergeOptions to redefine that icon in my top bar on every single screen in my app.

I have also tried with left buttons and by using an icon instead of a component for my buttons, but those seem to be broken as well when set as default options.

I traced it back to the v2.16.0-snapshot.270 release which lead me to this PR. That's when it was first introduced and it is also still an issue for me on the latest snapshot v2.17.0-snapshot.283.

vshkl pushed a commit to vshkl/react-native-navigation that referenced this pull request Feb 5, 2020
* Explicitly attach and detach viewControllers in store
* Stop registering components in commands handler
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.

3 participants