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

Fix multiple back buttons are required to navigate back. #2135

Closed
wants to merge 10 commits into from

Conversation

jainkuniya
Copy link
Member

So that it can override the hardware back button press implementation present in the TabNavigation of main tabs.

Fix: two hardware back button press are required to navigate back.
 So that main tabs state can be considered in stream tabs on
 hardware back button press.
So that hardware back button press can be handled in that.
Fix multiple back button press are required to navigate back
if all streams tab is selected in the stream tab of main tabs.
So that main tabs are not re-rendered when canGoBack changes.
Complete fix for multiple back button are required to navigate back.

Fix: zulip#2098
So that route can be popped out on back button from stack.
If current route is lighbox then also we can go back.
Created new selector isCurrentRouteIsLightbox, which
can be used to distinguish such cases and back arrow
button can be rendered accordingly.

Fix: zulip#2127.
Fix: multiple back button press are required if narrowed
from tab other home i.e steams and conversation.
Because one press was captured by main tab to navigate to
home tab that is the first tab.

Fix: zulip#2098
@zulipbot zulipbot added the bug label Mar 26, 2018
@zulipbot
Copy link
Member

Heads up @jainkuniya, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@gnprice
Copy link
Member

gnprice commented Mar 29, 2018

It looks like a couple of the steps in this PR are to add more layers between our different navigators -- specifically between the outer TabNavigator in MainTabs.js, and the TabNavigator that is StreamTabs.

In chat last week, after some very helpful comments from @HarshulSharma000 my conclusion was that we want to go the opposite direction: our inner navigators should be direct children (in screen properties) of the outer navigator, or of each other.

Do you disagree? Can you explain why? Perhaps that chat thread is the right place for further discussion.

@borisyankov
Copy link
Contributor

This was fixed in a simpler way.
@jainkuniya if you think parts of this PR are still relevant, do not hesitate to discuss them with us and open a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants