-
-
Notifications
You must be signed in to change notification settings - Fork 537
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
feat(Android): added navigationBarTranslucent option #2152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, few initial remarks.
Why is this a problem since 3.30
, why was the code removed that was responsible for this behaviour.
If we go with new prop, we do not restore backward compatibility and we commit to introducing breaking change in behaviour.
I want to see some description in PR why this is the way to go instead of restoring old behaviour and keeping backward compatibility.
Another issue is that we want to have similar behaviour on both platforms. How does this look like on iOS, does the problem occur there? How do we plan to handle this prop there?
@kkafar Right before 3.30 release this commit changed the navigation bar behavior. As far as I know @tboba feel free to share your thoughts. |
I think we need some elaboration on reasons & whys of the situation in the PR's description. This commit you refer to #1988 went under my radar & I'm not convinced it was good change. By adopting the suggested approach we would commit to a small, but still breaking change in behaviour & I'm not convinced we do need this. So basically I want you to understand & advocate for why #1988 was needed / or it was not, and then we can continue here. There is one more level to this change, how does this behave in context of edge-to-edge and not edge-to-edge applications? Also my questions related to iOS are still open. I'm picky today in my reviews 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good overall, but we need to have it properly documented in the PR description why is it needed, so the users know how to correctly use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks better, LGTM! Just one thing before merging 😄
android/src/main/java/com/swmansion/rnscreens/ScreenWindowTraits.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good, thank you! 🎉
There is one more thing, however, namely have you tested what happens on iOS (especially Paper, but check Fabric too) when somebody uses this prop? Won't we get any undesired behaviour?
@kkafar I've tested the prop on both fabric and paper and also checked to make sure it does not impact iOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided that iOS behaves nicely I think we're good to go
**Motivation** This PR intents to enable navigation bar translucency control. You can find more about this feature [here](software-mansion/react-native-screens#2152) **Test plan** Play around with following screen options: ``` <Stack.Navigator screenOptions={{ navigationBarColor: 'transparent', navigationBarTranslucent: true, navigationBarHidden: false, }}> ```
…n#2152) ## Description Once upon a time there was an [issue](software-mansion#1719) regarding navigation bar. The problem was that the background color set to the navigation bar covered the content behind, see examples: <table> <tr> <th>navigationBarColor: 'transparent'</th> <th>navigationBarColor: 'yellow'</th> </tr> <tr> <td> ![Screenshot 2024-05-22 at 10 33 28](https://github.com/software-mansion/react-native-screens/assets/91994767/eb3ea5a5-4617-4547-a052-ab0612872697) </td> <td> ![Screenshot 2024-05-22 at 10 34 08](https://github.com/software-mansion/react-native-screens/assets/91994767/805fa199-b90c-40aa-9ef5-b5ab8abef71a) </td> </tr> </table> Then [PR 1988](software-mansion#1988) solved the issue, but introduced another problem: sometimes it may be intended to have content behind the navigation bar. <table> <tr> <th>navigationBarColor: 'transparent' after PR 1988</th> <th>navigationBarColor: 'transparent' before PR 1988</th> </tr> <tr> <td> ![Screenshot 2024-05-21 at 13 00 23](https://github.com/software-mansion/react-native-screens/assets/91994767/76ec2fa1-e212-4725-a1da-32bcf8c5eb84) </td> <td> ![Screenshot 2024-05-21 at 13 07 19](https://github.com/software-mansion/react-native-screens/assets/91994767/8fa15e57-7168-4fec-b8e7-3cce29eeebdc) </td> </tr> </table> This PR intents to add a new screen option, enabling navigation bar translucency control, so that the developers can decide if they want to show content behind the navigation bar independently. Fixes software-mansion#2122 software-mansion#1719 ## Changes - added navigationBarTranslucent option ## Test code and steps to reproduce <!-- Please include code that can be used to test this change and short description how this example should work. This snippet should be as minimal as possible and ready to be pasted into editor (don't exclude exports or remove "not important" parts of reproduction example) --> ## Checklist - [ ] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes
Description
Once upon a time there was an issue regarding navigation bar. The problem was that the background color set to the navigation bar covered the content behind, see examples:
Then PR 1988 solved the issue, but introduced another problem: sometimes it may be intended to have content behind the navigation bar.
This PR intents to add a new screen option, enabling navigation bar translucency control, so that the developers can decide if they want to show content behind the navigation bar independently.
Fixes #2122 #1719
Changes
Test code and steps to reproduce
Checklist