Skip to content

Conversation

@ezanetta
Copy link
Contributor

@ezanetta ezanetta commented Jun 18, 2020

Resolves #754

Description:

  • Added style to FireDialog, and override some properties in order to fix this issue :)
  1. windowIsFloating as true
  2. navigationBarColor as ?toolbarBgColor (light/dark theme)
  3. statusBarColor as @android:color/transparent

Tested on:

  • Pixel XL api 29 emulator
  • Pixel 3XL api 29 emulator
  • Pixel 3a XL api 29 device
Old New
Screen Shot 2020-06-17 at 20 45 51 Screen Shot 2020-06-17 at 20 55 48

Live preview

live_demo


Internal references:

Software Engineering Expectations
Technical Design Template

@cmonfortep cmonfortep self-assigned this Jun 19, 2020
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great and it's simple to understand. I'm really looking forward for this fix!

I have only a comment related android:navigationBarColor, we should use it only for api > 27 to avoid some UI issues in low api devices.

Here a screenshot of how the bug looks like in api 21 (see that navigation buttons are also white and they are not visible):

Thanks!


<style name="FireDialog" parent="@style/Theme.Design.BottomSheetDialog">
<item name="android:windowIsFloating">false</item>
<item name="android:navigationBarColor">?toolbarBgColor</item>
Copy link
Contributor

@cmonfortep cmonfortep Jun 25, 2020

Choose a reason for hiding this comment

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

This produces a UI issue on low api devices. We had a similar issue in the past, where using white color makes all navigation buttons not visible. To avoid that bug I suggest you to create a new style for v27. I think we just need to keep this style here but without that attribute, and move this style to v27 style file.

Also, even if they are the same, I think is better to use colorPrimary instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't realize about that issue, sure, sounds good to me, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem at all, it also happened to us in the past. Happy you can work on this and make it happen 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed android:navigationBarColor from the main style, and move it to v27 file and now it's working great. I tried on api 29 / 21 / 23. I tried with colorPrimary but the result was @color/almostBlackDark instead of the actual primary color, I found another style on v27 using <item name="android:navigationBarColor">?toolbarBgColor</item> so I think it's ok? Let me know if we can improve anything 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I noticed we can avoid the property <item name="android:navigationBarColor">?toolbarBgColor</item> and the navigationBar works fine, the nav buttons are visible, but the nav background is always black (I think it takes the navBar system default color)

Copy link
Contributor

Choose a reason for hiding this comment

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

About using ?toolbarBgColor, if we are already using that color is ok 👍

And about about low api devices always being black is also ok, it's our expected behavior to avoid problems the problems I described with navigation buttons.

…tyle to v27 theme file for better compatibility
@ezanetta ezanetta requested a review from cmonfortep June 25, 2020 15:09
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

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

Thanks, code wise looks great. I'm going to approve it. I will merge it as soon as I do some final checks. I will let you know 👍

@cmonfortep cmonfortep merged commit 827f54b into duckduckgo:develop Jun 26, 2020
@cmonfortep
Copy link
Contributor

Thanks for the fix @ezanetta. This is now merged.

@ezanetta
Copy link
Contributor Author

Thanks for the fix @ezanetta. This is now merged.

Thanks Cristian, I'm glad I can help 😄

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.

Navigation buttons hidden on some devices

2 participants