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

Delete hideNavBar and replaced hideNavBar with child.hideNavBar #1406

Closed
wants to merge 3 commits into from
Closed

Delete hideNavBar and replaced hideNavBar with child.hideNavBar #1406

wants to merge 3 commits into from

Conversation

MechanicKim
Copy link
Contributor

@MechanicKim MechanicKim commented Nov 22, 2016

Please refer to #1363

As I know, default value of scene property 'hideNavBar' is false.
So, I fixed two lines in a file 'Defaultrenderer.js'

Added information!

  • To fix a problem about 'test' and 'jest', I updated some react packages.

@mention-bot
Copy link

@MechanicKim, thanks for your PR! By analyzing the history of the files in this pull request, we identified @joenoon, @andyschwob and @mat2maa to be potential reviewers.

@joenoon
Copy link
Collaborator

joenoon commented Nov 23, 2016

I'm a few versions behind and a bit out of the loop on this project, so I could be wrong here. But removing deepestExplicitValueForKey seems to me like it would break a lot of functionality that used to be demoed in the Example app: Go to tabbar > push new scene > and then all of the variations of hideTabBar hideNavBar that persisted through additional scenes and correctly recoiled as they were popped off. I would just urge someone more current on the state of the project to double check this. #1330 #1363

@MechanicKim MechanicKim mentioned this pull request Nov 23, 2016
9 tasks
@MechanicKim
Copy link
Contributor Author

@joenoon
I wanna help something as a person who uses this package.
what can I do for it?

@joenoon
Copy link
Collaborator

joenoon commented Nov 24, 2016

@MechanicKim If I'm guessing, I think what you have is something like this:

- root hideNavBar=false
  - scene1 hideNavBar=false
  - scene2 hideNavBar=true
  - scene3 (unspecified hideNavBar)

And when you go Actions.scene3(), you notice it thinks hideNavBar == true

If that accurately describes the issue, that was how I intended hideNavBar and hideTabBar to work - they are inherited values. The deepestExplicitValueForKey function walks the scene tree to get the inherited value when it isn't explicitly defined. Those props are special/different than other props to get the navigation behavior in the "push new scene" demo. I would compare it to how the ios UINavigationController behaves as you push and pop scenes while setting navigationBarHidden and hidesBottomBarWhenPushed as you go.

So I guess a few things:

  1. Am I describing the issue right?

  2. If so, it would not be a bug, so would better documentation resolve this? (either explicitly set hideNavBar on scenes, or at least set hideNavBar=false on any scene following a hideNavBar=true so it goes back to false).

  3. Or is this not a behavior people want? For my app this functionality is critical and everywhere, but I can work around it if the core library wants to remove it.

Also, I mentioned before I'm a few versions behind, so I'm kind of jogging my memory on this, and I can't vouch that this functionality hasn't been broken in the latest versions that I haven't seen yet.

@MechanicKim
Copy link
Contributor Author

First, I'm sorry. I might not know the intention of this react native package well.
To build our application, we are using this package. I just felt strange about using navigation bar.
For example, some html form tags have their default prop values. If we don't change it, they keep the default value. I have been used to this way.

@joenoon
Copy link
Collaborator

joenoon commented Nov 24, 2016

@MechanicKim no need to be sorry! What do you think about the rest? Does my explanation solve your issue or is there still something that doesn't work for your app?

@MechanicKim
Copy link
Contributor Author

MechanicKim commented Nov 25, 2016 via email

Copy link
Collaborator

@charpeni charpeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we can't update react-native without running react-native upgrade.

I fixed the CI in #1422, but if you want to update the example project you can do it in another PR and don't forget to run the upgrade command.

@charpeni
Copy link
Collaborator

Thank you @MechanicKim, your time is greatly appreciated.

I'll close this PR since it seems resolved, if you think it shouldn't be closed, please let me know.

@charpeni charpeni closed this Nov 26, 2016
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.

4 participants