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

SetStyle fix #2539

Merged
merged 3 commits into from
Jan 21, 2018
Merged

SetStyle fix #2539

merged 3 commits into from
Jan 21, 2018

Conversation

yogevbd
Copy link
Collaborator

@yogevbd yogevbd commented Jan 16, 2018

@danielang Removing changes in RCCNavigationController fixed the issue we had. Does this make sense?

@yogevbd yogevbd changed the title fixes #2524 SetStyle fix Jan 16, 2018
@danielang
Copy link
Contributor

danielang commented Jan 16, 2018

@yogevbd: I've add this changes because if you don't update all viewControllers in the stack of the screen they won't update their styles when you pop them: the tabbar stays in their previous color, etc.

@yogevbd
Copy link
Collaborator Author

yogevbd commented Jan 16, 2018

@danielang The problem is that all viewControllers in UINavigationController stack will call updateStyle on setStyle. It causes styles that affect UINavigationBar to be applied multiple times (like navBarHidden).
I think moving tabBarItem styles to setStyleOnInit will solve this.

@danielang
Copy link
Contributor

@yogevbd: by the naming convention of the method setStyleOnInit I thought that this method would only called once while the viewController is initializing.
So how the viewController will be notified when the style of the tabbar has been changed?

@danielang
Copy link
Contributor

@yogevbd any reaction? 🙂

@guyca
Copy link
Collaborator

guyca commented Jan 18, 2018

The feature your PR implemented is updating BottomTabs (tabBar) style by calling setStyle - @danielang can you confirm this PR covers the use cases you had in mind?

@danielang
Copy link
Contributor

@guyca: Yes, of cause.
But it seems that @yogevbd has discovered an 'bug' in iOS that the naviationbar will be updated too often while transitioning between screens; so it produces some lags.
I couldn't verfiy it in my simulator. And we've began a discussion about a better implementation. And now I am waiting for a response.

@yogevbd
Copy link
Collaborator Author

yogevbd commented Jan 18, 2018

@danielang What I had in mind is to implement another function that handled all tabItem styles and call it every time just like setStyleOnInit.

Copy link
Contributor

@danielang danielang left a comment

Choose a reason for hiding this comment

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

Sry for all the discussion above.
Your fix seems to work very good! I'ven't recognized your changes on my phone.
But now I've tried it in my app and thy work damn good. 😄

@yogevbd yogevbd merged commit 883dcfe into master Jan 21, 2018
@guyca guyca deleted the setStyle branch January 21, 2018 09:56
chilinh added a commit to chilinh/react-native-navigation that referenced this pull request Jan 22, 2018
* r_master:
  SetStyle fix (wix#2539)
  Update ISSUE_TEMPLATE
  fix(start app): show red screen in case an error is thrown while starting the app (wix#2556)
  Xcode 8 fixes
  fixed swizzle in xcode 8
  Back-port insets fix to v1
  Solved doc formatting issues (wix#2544)
  Update README.md (wix#2537)
  Update styling-the-navigator.md
  Adding preferredContentSize (wix#2529)
  Revert "setStyle to style the according TabBar (wix#2524)"
  setStyle to style the according TabBar (wix#2524)
yogevbd added a commit that referenced this pull request Jan 23, 2018
chilinh added a commit to chilinh/react-native-navigation that referenced this pull request Jan 25, 2018
* r_master:
  Extend SafeAreaView to support iOS 10
  Revert "SetStyle fix (wix#2539)"
  Enable Swipedown to dismiss on bottom aligned notification iOS (wix#2567)
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.

None yet

3 participants