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

Add barTintColor and tintColor to NavBarIOS and TabBarIOS #263

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mpiannucci
Copy link

commented Jul 12, 2019

More styles could be added in the same way down the road.

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 12, 2019

I just realized I need to expose the unselectedBarTintColor for TabBarIOS so I’ll do that tomorrow

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 12, 2019

The one last thing is that the style props are now applied in viewWillAppear but that gives a super rigid transition. I think using willMoveToSuperView might give a better transition.

@grahammendick

This comment has been minimized.

Copy link
Owner

commented Jul 13, 2019

Hey, thanks for putting the PR together! Is it ready for review because I couldn't get it to build? I think you mean _tabBarController instead of self.tabBarController. And there's no setter for the isTranslucent property. Sorry if I looked at it too soon. Thanks again

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 14, 2019

Sorry about that, I think I forgot to push my last updates. I’ll fix those and get back to you after

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 14, 2019

Okay should be all set now! Let me know what you think, its all pretty basic

@mpiannucci mpiannucci changed the title Add barTintColor and tintColor to NavBarIOS and TabBarIOS [WIP] Add barTintColor and tintColor to NavBarIOS and TabBarIOS Jul 16, 2019

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

This causes #265

@grahammendick

This comment has been minimized.

Copy link
Owner

commented Jul 16, 2019

Nice catch! I'll hold off reviewing this until you're happy

@mpiannucci mpiannucci changed the title [WIP] Add barTintColor and tintColor to NavBarIOS and TabBarIOS Add barTintColor and tintColor to NavBarIOS and TabBarIOS Jul 16, 2019

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 16, 2019

Okay sorry about all the issues, this should be good to go now. I got overzealous by including the translucency settings because they change the render surface. For now, I'll just leave them as default.

@grahammendick

This comment has been minimized.

Copy link
Owner

commented Jul 17, 2019

Thanks so much for the PR. Please don’t be put off by my long review comments. I like to give quite a bit of detail to help us arrive at a shared understanding of the feature.

Resetting Defaults

These tintColors are tricky because they live on the navigationBar on the UINavigationController and not on the navigationItem on the .UIViewController I think this is a design flaw of the iOS platform. It causes problems because properties on the navigationBar apply to all screens, unlike properties on the navigationItem which are isolated to the screen you’re in.

Take the example of a navigation from A → B. Let’s say that Screen A doesn’t set the barTintColor and that Screen B sets it to green. When Screen A loads it has the default color but after navigating to Screen B and then back, Screen A has the color green! That’s because the settings from B were made to the shared navigationBar.

To fix this, we must set the default barTintColor if the prop is null. We must do that in both the NVSceneController as well as in NVNavigationBarView.

Text Attributes

It was interesting to see you set the color of the titles using the TextAttributes (I never knew about them before). There are quite a lot of TextAttribues, not just color. I don’t think we want to introduce new props for each of them. I had a look at how React Native handles them for its Text component. It uses the style prop instead of separate props for each of them. Couldn’t we do the same?

<NavigationBarIOS barTintColor=greenstyle={{color: 'white'}} />

Then the rest of the TextAttributes become extra style props

<NavigationBarIOS barTintColor=greenstyle={{color: 'white', fontSize: 20}} />

And do we even need barTintColor at all?

<NavigationBarIOS style={{backgroundColor: 'green', color: 'white', fontSize: 20}} />

I got a bit lost in the React Native code. Can you work out how it uses the TextAttributes class? Again, we must reset the defaults because you can set the text color and then clear it.

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 17, 2019

Resetting Defaults

Agreed this is important. Another part of it is that usually an app will want to have a default barTintColor for the app which can be overridden in each Screen. That also means there should be default style for the title as well most likely? What do you think? Cuz the hardest part of that is just making sure there are known defaults to revert to when navigating away from a screen.

Text Attributes

So I looked into this a bit. In addition to the cpp version you linked, there is alos the objective c version. Then if you trace back the Text view you get to the shadow view where the attributes are actually computed and applied here. They are exposed to JavaScript using the BaseViewManager.

So I think we can do something just like this but just simpler. Ill play around with it when I get a chance.

@grahammendick

This comment has been minimized.

Copy link
Owner

commented Jul 17, 2019

Won't setting barTintColor to nil reset to the default?

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 17, 2019

It might, I have never tried. I was also saying that you could expose a wayt o customize what the default is so you dont have to set the color in every single screen unless you want to override it. Does that make sense?

@grahammendick

This comment has been minimized.

Copy link
Owner

commented Jul 17, 2019

I'm not keen on global defaults because it seems to go against the React grain. I think we should stick to just components and props and leave it up to the user to devise their own approach to theming. For example, they could add their Theme to React Context

const theme = useContext(ThemeContext);
<NavigationBarIOS style={{color: theme.color} />
@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 17, 2019

Fair enough! Ill play with the text styles soon

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 18, 2019

The style props work totally fine as expected. The hard part now is resetting to defualts. I tried to get this working but ht a problem where the props for the navbar are not set again when navigating back. Moving forward works fine becasue everything comes in the correct order so you can set the props, then set the nav bar property members to nil when the view navigates away, then the new props are passed down for the next view. But when you click the back button, the original navbar props are not passed again, they are expected to have persisted in the NavBar.

I assume this can probably be handled but I am not super familiar with the NavigationStack itself yet.

@grahammendick

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

When you navigate back from B to A, iOS calls viewWillAppear on Screen A. This is where you reset the navigation bar style from the NVNavigatonBarView properties. I think it's enough to remove the null checks from viewWillAppear.

@mpiannucci

This comment has been minimized.

Copy link
Author

commented Jul 18, 2019

Coulda sworn that was the first thing I tried. It works fine but I have to find a way to make the transition between bar colors more smooth when navigating back.

@grahammendick

This comment has been minimized.

Copy link
Owner

commented Jul 18, 2019

Have you tried viewDidAppear instead of viewWillAppear?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.