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

[Android] Add support for translucent header #575

Merged
merged 9 commits into from
Sep 29, 2020

Conversation

andreibarabas
Copy link
Contributor

Currently, the headerTranslucent = true is only supported on iOS. With this PR the setting will also work on Android

@kyle-ssg
Copy link
Contributor

kyle-ssg commented Jul 24, 2020

Ah awesome! I Guess that would resolve #573. Just going to try this out.

@kyle-ssg
Copy link
Contributor

kyle-ssg commented Jul 24, 2020

@andreibarabas This works exactly as I'd expect except when popping, it seems to force everything to render after the navbar.

@andreibarabas
Copy link
Contributor Author

Hey @kyle-ssg - i'll take a look

@WoLewicki
Copy link
Member

I would love to merge it! Please inform me when it is ready for review. It should also close #405.

@WoLewicki WoLewicki linked an issue Jul 30, 2020 that may be closed by this pull request
@andreibarabas
Copy link
Contributor Author

Hey @WoLewicki , I need a couple more days to get to this. I'm currently caught up in a release, and once that is done can look more into this.

@WoLewicki
Copy link
Member

@andreibarabas it is not a problem 🙂

@WoLewicki
Copy link
Member

How are things going on this PR @andreibarabas? Do you need any help or you just didn't have time to finish it?

@andreibarabas
Copy link
Contributor Author

Haven't yet had the time to dive deep into it, but will give it another shoot hopefully tomorrow / the day after that.

I also need this fixed for our production build, so top priority ;)

@andreibarabas
Copy link
Contributor Author

@WoLewicki - done ;) it works now, tested it with pop also.

@WoLewicki WoLewicki linked an issue Aug 31, 2020 that may be closed by this pull request
@WoLewicki
Copy link
Member

Looks good! There are some other places where the information about the platform availability should be changed: https://github.com/software-mansion/react-native-screens/blob/master/README.md#L194, https://github.com/software-mansion/react-native-screens/blob/master/src/index.d.ts#L158, https://github.com/software-mansion/react-native-screens/blob/master/src/native-stack/types.tsx#L116. If you could change this, I would be grateful. Also, do you know of any options to have something like contentInsetAdjustmentBehavior for Android? Would be good to have consistent behavior for ScrollViews on both platforms.

@kyle-ssg
Copy link
Contributor

kyle-ssg commented Aug 31, 2020

Hey @andreibarabas, from some brief testing I've noticed the title isn't rendering at least for me, the left is iOS.
image

@andreibarabas
Copy link
Contributor Author

Hey @kyle-ssg . Title i have't checked. Our design system doesn't use them.

Let me check that and get back.

Any other issues?

@kyle-ssg
Copy link
Contributor

Hey @kyle-ssg . Title i have't checked. Our design system doesn't use them.

Let me check that and get back.

Any other issues?

Other than that it looks awesome @andreibarabas!

@andreibarabas
Copy link
Contributor Author

@kyle-ssg - can you provide a snack maybe? I checked on our app and put a dummy title and it showed up. Here it is
Screenshot_20200915_094510

@WoLewicki - updated the doc as per your suggestion.

As for contentInsetAdjustmentBehavior, I'm not really an android developer, so would have to deeply research it. maybe in a different PR? Do you see that as blocking for this one?

@WoLewicki
Copy link
Member

@andreibarabas I don't think it is blocking since this transparent header is an optional thing anyway.

@kyle-ssg
Copy link
Contributor

Yeah, happy with that. I'll raise separately with repro

README.md Outdated Show resolved Hide resolved
@andreibarabas
Copy link
Contributor Author

hey @WoLewicki - anything else you guys need to have this merged?

@WoLewicki
Copy link
Member

Didn't @kyle-ssg have problems with the title that needed to be resolved? Or are they gone?

@andreibarabas
Copy link
Contributor Author

I can't repro them. All screenshots I sent above include the title

@brunocrpontes
Copy link

brunocrpontes commented Sep 21, 2020

I tested this branch in a personal app and headers title just go away when use an headerLeft component in bar.
Maybe it can be the same case of @kyle-ssg.

@WoLewicki
Copy link
Member

@brunocrpontes is it specific to translucent header? I think you are talking about this behavior: https://github.com/software-mansion/react-native-screens/tree/master/native-stack#headerleft and the same happens with normal header. Am I right?

@brunocrpontes
Copy link

@brunocrpontes is it specific to translucent header? I think you are talking about this behavior: https://github.com/software-mansion/react-native-screens/tree/master/native-stack#headerleft and the same happens with normal header. Am I right?

Yes, you're right.

@WoLewicki
Copy link
Member

@kyle-ssg so can you provide a repro showing the lack of title? I would like to test it and merge it if all is ok.

@kyle-ssg
Copy link
Contributor

kyle-ssg commented Sep 22, 2020

Hey @WoLewicki sorry today I can't really busy release, it would make sense if it's the custom header left that's doing it though. I bet that is the usecase!

Edit: I can confirm this does not happen when there is no headerLeft custom component. So replication is simply have a screen where theres a custom header left and a title with transparent nav.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overal, I only have one question I left inline regarding one change. I'd like to get that one item clarified, and this is good to merge otherwise

@@ -238,7 +242,7 @@ public void onUpdate() {
}

// background
if (mBackgroundColor != 0) {
if (mBackgroundColor != -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this change is made here? If we expect "default" to be -1 maybe we should also set the default in @ReactProp while defining bgColor:

@ReactProp(name = "backgroundColor", customType = "Color", defaultInt = -1)

??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is a good idea to add that for extra safety.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. will take care of it as soon as possible.

There is still an open issue from @kyle-ssg which reproduces only if there is a custom left component. I won't have time earlier than next week to investigate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think this is not an issue since it is the expected behavior for the title to disappear when there is headerLeft component.

Copy link
Contributor

@kyle-ssg kyle-ssg Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of if the headerLeft occurred before / after this work I think this is good to merge, I'll look into this separately and raise something if I think there's an issue, it does seem a bit weird to me (mainly because im majority iOS) though it's definitely not a blocker. @WoLewicki @andreibarabas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On iOS, the situation is kind of similar but for headerCenter, where using it makes the title disappear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. it's fine with me. I added @kmagiera suggestion, so let me know if you need anything else from my side to get this merged.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am testing these changes out and this line seems to have introduced a bug where I can't set the header background color to "#fff" or "#ffffff".

@WoLewicki
Copy link
Member

Thanks for your contribution! The only problem I can spot is that passing a wrong color name is resolved to 0 so the header becomes transparent, but I think we can classify it as a problem of the programmer passing wrong options. I am merging it then!

@WoLewicki WoLewicki merged commit 413469c into software-mansion:master Sep 29, 2020
@ReactProp(name = "backgroundColor", customType = "Color")
public void setBackgroundColor(ScreenStackHeaderConfig config, int titleColor) {
config.setBackgroundColor(titleColor);
@ReactProp(name = "backgroundColor", customType = "Color", defaultInt = -1)
Copy link

@andrewbeckman andrewbeckman Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreibarabas I am noticing that when I try to set my header background color to "#fff" or "#ffffff" (i.e. white), I end up with a non-white header. If you take a look at the attached photo, you can see the header appears to be gray.

Any chance this bug came in with the changes to this file?

Screen Shot 2020-10-04 at 12 12 00 PM

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, this is when headerTranslucent is set to false.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some further testing, @andreibarabas it looks like it was the change in ScreenStackHeaderConfig.java

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the "#fff", "#ffffff" and white colors all have value of "-1" on the native side, so the newly added if makes it impossible to use these values. Can you check if #656 fixes the issue?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WoLewicki that fixed the issue! Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guys, glad you solved it. Thanks! Just now had the time to groom the notifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants