Feature/styling #307
Feature/styling #307
Conversation
* fix/android-tab-color-crash: Fixed crash occurring when tabBarBackgroundColor or tabBarButtonColor is not supplied
# By Guy Carmeli # Via Guy Carmeli * 'master' of github.com:wix/react-native-navigation: 2.0.0-experimental.76 Show modal with animation
# By Guy Carmeli # Via Guy Carmeli * 'master' of github.com:wix/react-native-navigation: 2.0.0-experimental.78 Fix npe in BottomTabswhen appStyle is undefined. Again. 2.0.0-experimental.77 Fix npe in BottomTabswhen appStyle is undefined Conflicts: android/app/src/main/java/com/reactnativenavigation/views/BottomTabs.java
# By Guy Carmeli (8) and Daniel Zlotin (7) # Via Guy Carmeli * 'master' of github.com:wix/react-native-navigation: 2.0.0-experimental.86 Use leftButtons api on Android like in iOS 2.0.0-experimental.85 fixed drawer animation warning on iOS 2.0.0-experimental.84 fixed wix#66 2.0.0-experimental.83 fix killed in background bug && update package.json Revert "Call recreate instead of finish and startActivity" 2.0.0-experimental.82 2.0.0-experimental.81 Fix screen style in modal's initial screen 2.0.0-experimental.80 2.0.0-experimental.79 Fix bottomTab not getting correct label on Android
# By Guy Carmeli # Via Guy Carmeli * 'master' of github.com:wix/react-native-navigation: 2.0.0-experimental.87 Fix push to modal results in invisible modal
# By Guy Carmeli # Via Guy Carmeli * 'master' of github.com:wix/react-native-navigation: Dismiss all modals in correct order Clarify unsupported left button exception message Clarify unsupported left button exception message
Conflicts: android/app/src/main/java/com/reactnativenavigation/react/NavigationReactGateway.java
…ush another view controller
…rollers and fixes setStyle mutating colours
|
Very good work! I want to review this carefully before merging, as it is a large PR. One thing immediately apparent is your use of the local copy of controllers. This requires more changes on our end so I'll merge this after we finish with the controllers merge (you'll probably need to rebase) |
|
Thank you! Of course yeah, a lot of it is just spacing changes as Xcode didn't like your indentation for some reason! Yes, I noticed that last minute, wasn't sure about changing it, but more than happy to rebase once you guys are done with that! For now we'll just build from our fork :) |
Pulls latest changes from wix repo
|
@DanielZlotin Looks like you guys have now finished the controllers merge? I've merged master back into my branch so it is up to date with your codebase :) |
* feature/rn33: Fixes subtitle being shown Fixes updating title font styling with new RCCTitleViewHelper logic Fixed to work with RN 33 Changes import to reference controllers rather than react-native-controllers and fixes setStyle mutating colours Makes sure autoAdjustScrollViewInsets isn't propagated through when push another view controller Adds ability to set modal presentation style Adds some styling to tab bar controller and fixes some bugs Fixes setting style on navigator Fixes setStyle function Adds native code for setting style on RCCViewController Adds setStyle property to index.js Conflicts: android/app/src/main/java/com/reactnativenavigation/react/ReactGateway.java src/deprecated/platformSpecificDeprecated.ios.js
|
Thanks, I'll review it soon |
|
@DanielZlotin It would be really great if we could get this looked into being merged in soon. We're currently doing quite a lot of active development on our fork, which would be great to get pull-requested in, but it's all based off of this branch for us and is starting to become a PITA keeping up to date with your master branch! |
|
I've just seen that it also seems like you're slowly adding stuff in which we already have implemented here (Not sure if those were separate pull requests, or yourself developing them), but kinda seems a bit pointless :P |
|
@simonmitchell There was some brief chatter about this on the react native discord channel… I was also asking whether this PR would be looked into any time soon. but response was, that this will not be merged as it is unreasonably big and it would not be feasable to go over such a PR :/ |
|
:/ Well I guess we won't be pull-requesting any future work built on top of this then, that's a shame :( Are the owners aware a substantial amount of this is simply changing indentation? Edit: @florianbepunkt thanks for flagging that with me, I'll check out the discord channel |
|
Can't speak to that but I mentioned exactly this on Discord. Better talk to them directly… personally I would really look forward to this PR being merged. |
# Conflicts: # src/deprecated/platformSpecificDeprecated.android.js
|
Sorry for the confusion here @DanielZlotin and @guyca! Was nice chatting to you in Discord last night and hearing more about your processes! I've updated the pull request to include your latest changes on master, and will endeavour to continue to do so until you guys are ready to take it on! :D |
| @@ -90,6 +90,7 @@ public void onJsDevReload() { | |||
|
|
|||
| private ReactInstanceManager createReactInstanceManager() { | |||
| ReactInstanceManager.Builder builder = ReactInstanceManager.builder() | |||
| .setUseOldBridge(true) | |||
There was a problem hiding this comment.
Is this still needed? If not, let remove it.
There was a problem hiding this comment.
Just confirmed with my colleague, he can't see any reason that it's still needed. Shall I remove it?
There was a problem hiding this comment.
Hiya @guyca, I've tried editing this both locally on my machine and in GitHub, but can't seem to see this line in the file that is referenced above: android/app/src/main/java/com/reactnativenavigation/react/NavigationReactGateway.java am I being an idiot? :P
# Conflicts: # src/deprecated/platformSpecificDeprecated.android.js
|
@guyca Apologies for the lateness on fixing, but just sorted the merge-conflict here! Had Friday off work so now was the first chance I've had! |
|
@simonmitchell I try to merge your code, but it doesn't compile, it failed to find Can u please make it compile in order to continue the review? Thanks in advance. |
|
Apologies, we're working on this by cherry-picking from a similar branch in our repo, so haven't given it a run in a while! Will make sure it's all up to scratch and compiling now :D |
|
Should be compiling again fine now @gran33 👍 |
|
@Antonides Is your offer for Android implementation still relevant? 🙏 |
|
@simonmitchell After these changes, I am not able to set the subtitle color in iOS. I see that the previous property is not read anymore and I am not sure what is the new way of setting it. Could you please provide a list of things that have changed (breaking changes) in this PR so other users can be aware of it and change/test their code accordingly. Thanks! |
|
We had the similar issues as mentioned in #418 and looked into it a little deeper. While investigating we found that the line causing this issue (not being able to push, after having called So our question is simple @simonmitchell; Why was this line added? What does it achieve? We tried removing it out and that makes it work as intended, but are we breaking anything by removing this line? And why was this merged in during a feature/styling branch? |
|
@varungupta85 Apologies I completely missed this notification! When I get a chance I will try and remember to have a look at doing this. @kasperkronborg I don't believe I added that line myself. When we were working on this Pull Request (For our client RFU, whom we required custom fonts e.t.c. in the navigation bar) we had to keep the It looks like that line of code was already present when @DanielZlotin copied the code over from the old Repo (react-native-controllers) to here: 18b1e8f |
|
@simonmitchell I found some unwanted behavior. The |
|
Fixed the subtitle-title issue in #1847 |
Adds better styling:
N.B:
will not work in RN < 0.33, however it is a simple change to fix it if you don't want to support 0.33 yet (Everything else seems to work find in 0.33)