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

navigator.getCurrentRoutes #553

Closed
wants to merge 3 commits into from
Closed

navigator.getCurrentRoutes #553

wants to merge 3 commits into from

Conversation

Kureev
Copy link
Contributor

@Kureev Kureev commented Mar 31, 2015

According to our talk with @ericvicenti about renderScene arguments

@ericvicenti
Copy link
Contributor

So, I've thought about this, and I am concerned because this navState will sometimes appear to be incorrect in the renderScene prop.

Here's the simplest example:

  • Initialize navigator with one scene
  • Push on a new scene
  • The navState and the routeStack that was passed to the first renderScene is now out-of-date, but we don't want to re-render it because it is going offscreen

I think this would probably add more confusion. Instead, I want to add a method to Navigator that lets you get the current routeStack.

Is there a reason you need to access the routeStack from inside the navigator? Maybe I could show you where you might run into some issues and help you find an alternative.

cc @ide , we were talking about this earlier

@Kureev
Copy link
Contributor Author

Kureev commented Apr 3, 2015

@ericvicenti Thanks for your feedback! Yeah, I have a use-case for that: for example, I created a navbar component I wanna use with Navigator. As far as I'm not sure how navigationBar property supposed to work (position at the bottom in the render stack is really confusing me. I don't wanna add position: absolute, top: 0, left: 0 to every view). You can check the stub I made in my component to support this. Probably, it's a good idea to expose getRouteStack()-like endpoint as third parameter for renderScene?

@ericvicenti ericvicenti changed the title Navigator's renderScene now accepts state instead of ref navigator.getCurrentRoutes Apr 3, 2015
@ericvicenti
Copy link
Contributor

I don't want to pollute renderScene's arguments. Thanks for adding it to the navigator! This looks good, I'll start merging this in.

We can come back to the navBar ordering issue later. Absolute positioning of the navBar is OK because it needs to appear over the scenes in most cases. You shouldn't need to apply position: absolute, top: 0, left: 0 to every scene; have you tried flex: 1?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 7, 2015
@Kureev
Copy link
Contributor Author

Kureev commented Apr 12, 2015

Is it going to be merged some day?

@vjeux vjeux closed this in a582d67 Apr 13, 2015
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 14, 2015
Summary:
According to our talk with @ericvicenti about `renderScene` arguments
Closes facebook#553
Github Author: Kureev Alexey <[email protected]>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 15, 2015
Summary:
According to our talk with @ericvicenti about `renderScene` arguments
Closes facebook#553
Github Author: Kureev Alexey <[email protected]>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020
* Promote 0.57 to stable and add back out-of-tree-platforms

* Prettier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants