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

feat(Android): added navigationBarTranslucent option #2152

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented May 21, 2024

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:

navigationBarColor: 'transparent' navigationBarColor: 'yellow'

Screenshot 2024-05-22 at 10 33 28

Screenshot 2024-05-22 at 10 34 08

Then PR 1988 solved the issue, but introduced another problem: sometimes it may be intended to have content behind the navigation bar.

navigationBarColor: 'transparent' after PR 1988 navigationBarColor: 'transparent' before PR 1988

Screenshot 2024-05-21 at 13 00 23

Screenshot 2024-05-21 at 13 07 19

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

  • added navigationBarTranslucent option

Test code and steps to reproduce

Checklist

Copy link
Member

@kkafar kkafar left a 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?

@alduzy
Copy link
Member Author

alduzy commented May 21, 2024

@kkafar Right before 3.30 release this commit changed the navigation bar behavior.

As far as I know WindowCompat.setDecorFitsSystemWindows() caused problems with screen layout and/or jumps in some cases, so it's not a good idea to add the logic back in all cases when navigation bar is not hidden.
In certain situations users may want to achieve such appearance and I think it's best to enable it through a separate option.

@tboba feel free to share your thoughts.

@alduzy alduzy requested a review from kkafar May 21, 2024 14:10
@kkafar
Copy link
Member

kkafar commented May 22, 2024

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 😅

Copy link
Member

@WoLewicki WoLewicki left a 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.

@alduzy alduzy requested a review from WoLewicki May 22, 2024 09:07
Copy link
Member

@tboba tboba left a 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 😄

Copy link
Member

@kkafar kkafar left a 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?

@alduzy
Copy link
Member Author

alduzy commented May 28, 2024

@kkafar I've tested the prop on both fabric and paper and also checked to make sure it does not impact iOS

@alduzy alduzy requested a review from kkafar May 28, 2024 14:24
Copy link
Member

@kkafar kkafar left a 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

@alduzy alduzy merged commit 0e62266 into main Jun 3, 2024
6 checks passed
@alduzy alduzy deleted the @alduzy/navigation-bar-translucent branch June 3, 2024 14:15
satya164 pushed a commit to react-navigation/react-navigation that referenced this pull request Jun 20, 2024
**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,
    }}>
```
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.

Inconsistency with navigationBarColor Transparency on react-native-screens versions >= 3.30.0"
4 participants