Bottom Navigation #778
Bottom Navigation #778
Conversation
…the content, but shows it when scrolling up
…crolling for that
# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
# Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt # app/src/main/res/layout/fragment_browser_tab.xml
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
We are getting close to finishing this, it's looking very good. GJ
About testing:
- ❌New tab pixel not triggered when using bottom bar, it works for the control group
- Question: why all the icons from the bottom bar fire pixels except the tab switcher button?
| } | ||
|
|
||
| private fun openInNewBackgroundTab() { | ||
| appBarLayout.setExpanded(true, true) |
There was a problem hiding this comment.
Noticed that appBarLayout.setExpanded(true, true) was here to show/expand the appBarLayout when the user opens a new tab in the background.
The reason may be to show the appBar and make the tab counter animation visible to the user to let them see a new tab has been created. For the Bottom Bar variant, we show the appbar too but the tab counter is not there. Have you considered showing the bottombar in that variant too?
There was a problem hiding this comment.
setExpanded is not used to make the counter animation visible, but to make the whole appBar visible. There are cases where the appBar will be hidden (the user is scrolling down and opens a new tab in the background). In that case we want to make sure that the appBar is visible, so we expand it.
There was a problem hiding this comment.
Discussed this offline and agreed that we should show the bottom bar when opening a background tab to make the tab icon visible so the animation is performed in the foreground.
| val view = tabsButton?.actionView as TabSwitcherButton | ||
| view.increment { | ||
| tabsButton?.increment { | ||
| addTabsObserver() |
There was a problem hiding this comment.
are we adding two observers each time we open a background tab?
Maybe we should check if experiment is enabled? (maybe also part of the decorator?)
There was a problem hiding this comment.
I like that, good call
| configureKeyboardAwareLogoAnimation() | ||
| configureShowTabSwitcherListener() | ||
| configureLongClickOpensNewTabListener() | ||
| configureAppBar() |
There was a problem hiding this comment.
this method is already called at line 344.
| } | ||
| } | ||
|
|
||
| private fun configureAppBar() { |
There was a problem hiding this comment.
I think I already commented something similar (maybe it was another method), but I think appbarnot only contains the privacyGrade, so naming is confusing here. Maybe we should change it after to configurePrivacyGrade instead.
| inner class BrowserTabFragmentDecorator { | ||
| fun decorateToolbar(viewState: BrowserViewState) { | ||
| if (variantManager.getVariant().hasFeature(VariantManager.VariantFeature.BottomBarNavigation)) { | ||
| decorator.hideToolbarMenu() |
There was a problem hiding this comment.
decorateAppBarWithBottomBar already does a menuButton?.gone(). And based on the checked condition + not using viewState, I think we can remove this method call.
There was a problem hiding this comment.
good call, it's not necessary
| } | ||
|
|
||
| lastSeenBrowserViewState?.let { | ||
| decorator.decorateToolbar(it) |
There was a problem hiding this comment.
This method is already called inside renderToolbarMenus.
|
|
||
| companion object { | ||
|
|
||
| private const val margin = 30 |
There was a problem hiding this comment.
const names should be in uppercase?
| android:viewportHeight="24"> | ||
| <path | ||
| android:pathData="M16,4C16.8284,4 17.5,4.6716 17.5,5.5V18.6667L12.9459,16.2378C12.3546,15.9225 11.6452,15.9226 11.0541,16.2378L6.5,18.6667V5.5C6.5,4.6716 7.1716,4 8,4H16ZM8,2C6.067,2 4.5,3.567 4.5,5.5V20.3333C4.5,21.0883 5.3045,21.5709 5.9706,21.2157L11.9953,18.0025C11.9982,18.0009 12.0018,18.0009 12.0047,18.0025L18.0294,21.2157C18.6955,21.5709 19.5,21.0883 19.5,20.3333V5.5C19.5,3.567 17.933,2 16,2H8Z" | ||
| android:fillColor="@color/bottom_bar_icon_color_selector"/> |
There was a problem hiding this comment.
To avoid future problems, if this color is tied to the bottom bar, icon name should reflect that it is expected to be used in the bottom bar too. Otherwise, the color can be generic maybe.
Same applies to rest of icons.
There was a problem hiding this comment.
Not to all the icons, I've renamed the ones that only apply to the Bottom Bar. Fire and Overflow icons are used in the control group and shouldn't be renamed.
| android:layout_marginStart="7dp" | ||
| android:layout_marginEnd="7dp" | ||
| android:clipToPadding="false" | ||
| android:paddingTop="40dp" |
There was a problem hiding this comment.
I think I alredy commented this, but this paddingTop seems not correct. There's too much space at the top of the screen
There was a problem hiding this comment.
These are the design specs in Figma and already reviewed by Design
build.gradle
Outdated
| } | ||
| dependencies { | ||
| classpath 'com.android.tools.build:gradle:3.6.2' | ||
| classpath 'com.android.tools.build:gradle:3.6.3' |
There was a problem hiding this comment.
I think, this is not related to this PR
…duckgo/Android into feature/david/bottom_navigation_bar
If you follow the discussion here https://app.asana.com/0/1157893581871903/1168245052250051 you'll see that the Tab Switcher was not considered a top level action and therefore we didn't feel the need to pixel it. In the end, we want to compare the engagement for top level actions and the tab switcher is not one of them @cmonfortep |
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
Yay! We did a great effort here, this looks good to me. Thanks @malmstein for the work here, I know it has been long and complex but I'm looking forward to seeing this released for our users.
|
Thanks for all the work @cmonfortep ! |
3 participants
Task/Issue URL: https://app.asana.com/0/488551667048375/1168086989855979
Pixels and Dashboard: https://app.asana.com/0/1157893581871903/1168245052250051
Product Changes: https://app.asana.com/0/1157893581871903/1170128420609770
Description:
Device comfort on our Android app is pretty bad - on larger devices it’s impossible to use our app with one hand and we get consistent (though low to mid frequency) around this.
Material design has bottom navigation options that we could leverage to improve device comfort and bring the navigation patterns closer to what we have on iOS, which is better when it comes to device comfort (with room to improve though).
I’d like to experiment around migrating the primary navigation (if not all of it) to the bottom of the screen to see what impact it has on retention.
Steps to test this PR:
Concept test variant mf: Control group
Concept test variant mm: Bottom Navigation
Steps to test Pixels
Each pixel event has a suffix that identifies either the control group or the experiment group.
mmmnm_nav_pm_o_mmorm_nav_pm_o_mnm_nav_f_p_mmorm_nav_f_p_mnm_nav_r_p__mmorm_nav_r_p_mnm_nav_nt_p_mmorm_nav_nt_p_mnm_nav_b_p_mmorm_nav_b_p__mnm_nav_s_p_mnScreenshots:
Light theme:
Dark theme: