Add support for hiding the Toolbar on Android#52
Conversation
|
@MattDavies Thanks a lot for the PR. I'm sure the community will appreciate it! |
| private static final String KEY_TOOL_BAR_STYLE = "navigatorStyle"; | ||
| private static final String KEY_STATUS_BAR_COLOR = "statusBarColor"; | ||
| private static final String KEY_TOOL_BAR_COLOR = "toolBarColor"; | ||
| private static final String KEY_TOOL_BAR_HIDDEN = "toolBarHidden"; |
There was a problem hiding this comment.
Lets change the text toolBarHidden to navBarHidden which is the property we already use in iOS
There was a problem hiding this comment.
Sure, easy. There are a few other "toolBar" props we can update in a separate PR too.
|
@guyca Changed to |
| mContext.runOnUiThread(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| updateToolbar(mOriginalScreen, mOriginalToolbar); |
There was a problem hiding this comment.
If the Modal is dismissed, the previous toolbar already has the correct style and buttons. Do we call updateToolbar only for mContext.setSupportActionBar(toolbar); ?
There was a problem hiding this comment.
Ah. I was finding the visibility of the previous toolbar was being altered and I needed to reset it, but that might have been before I was setting the action toolbar context correctly on the modal. Let me have a play and see if I can clean this up.
There was a problem hiding this comment.
Great. What do you think about moving updateToolbar to RnnToolbar class? I didn't have time to check this, but I think it will be used from other places as well.
Maybe something like this:
@UiThread
public void updateToolbar(Screen screen) {
setTitle(screen.title);
setSupportActionBar();
setStyle(screen);
setupToolbarButtonsAsync(screen);
}
public void setSupportActionBar() {
((AppCompatActivity) getContext()).setSupportActionBar(this);
}
Then, in RnnModal we would call mToolBar.updateToolbar(mScreen);
This also eliminates the need to keep a hard ref to mOriginalToolbar, a View that belongs to the previous Activity/Modal, which is a bad practice.
If we expose setSupportActionBar we also don't need to keep a ref to mOriginalScreen - seems like it simplifies the class a bit.
MattDavies
force-pushed
the
master
branch
2 times, most recently
from
3708dd9 to
ad56faf
Compare
| super.pop(navigatorId); | ||
| Screen screen = mScreenStacks.get(mCurrentStackPosition).pop(); | ||
| setNavigationStyle(screen); | ||
| setNavigationStyle(getCurrentScreen()); |
There was a problem hiding this comment.
When popping it was applying the style of the screen that had just been popped rather than the new screen.
| ModalController.getInstance().remove(); | ||
| super.onBackPressed(); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When pop was called on the only remaining screen on a modal it didn't appear to be clearing the modal out properly.
|
Hey @guyca I managed to simplify the modal logic a fair bit and fixed another couple of issues which I was hitting. I've got a couple of changes in https://github.com/MattDavies/react-native-navigation/tree/test-toolbar-hide which you might find useful for testing as well. |
MattDavies
force-pushed
the
master
branch
2 times, most recently
from
f0eafd9 to
1498ad8
Compare
| } | ||
|
|
||
| public void push(Screen screen) { | ||
| updateToolbar(screen, mToolBar); |
There was a problem hiding this comment.
It didn't look as though we were previously updating the toolbar style for a pushed screen within a modal.
There was a problem hiding this comment.
Good catch! We also miss this when pushing a screen in Activities (BottomTabActivity and SingleScreenActivity). That's why I suggested we declare updateToolbar in RnnToolbar.
|
@MattDavies I went over #60ab611a31a5b5681e236b4f7a8ca3f0a51f6622 and seems like you're still using If you wish I can move PS |
|
@guyca Updated to move I think I lost the |
| mToolbar.setTitle(initialScreen.title); | ||
| setSupportActionBar(mToolbar); | ||
| mToolbar.updateToolbar(initialScreen); | ||
| setNavigationStyle(initialScreen); |
There was a problem hiding this comment.
This still sets window styles so I left it there even though there's now a slight duplication. Lets look at that in another PR?
There was a problem hiding this comment.
Sure, we'll address that in a separate PR. I'll merge soon. Thanks for the contribution Matt 👍
There was a problem hiding this comment.
Welcome! Great library, it's making a huge difference to some of my apps compared to the official nav. I'll keep jumping in where I can to fill gaps.
Minor refactor after merging #52.
2 participants
This PR will ideally also make it a lot easier to support multiple Toolbar styles within a single app.
Most of the edge cases I could find were around multiple modals, but worth checking tab/single screen activity push/pop shows/hides the Toolbar correctly as well.
Haven't touched native Android code before, so really open to feedback and suggestions.