Feature/david/remove bottom bar #844
Conversation
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
Everything looks fine! Just a few things that we may want also to remove from the experiment and we are good to go.
| <com.duckduckgo.app.browser.ui.BottomNavigationBar | ||
| android:id="@+id/bottomNavigationBar" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="@dimen/bottomBarHeight" |
There was a problem hiding this comment.
pending to remove this dimen
| window.statusBarColor = Color.TRANSPARENT | ||
|
|
||
| setContentView(R.layout.activity_tab_switcher_bottom_bar_feature) | ||
| extractIntentExtras() |
There was a problem hiding this comment.
Should this file be totally removed? I see we removed it from the manifest and it's related to the BottomBar experiment. Also activity_tab_switcher_bottom_bar_feature.xml + layout_tabs_bottom_navigation_bar.xml
There was a problem hiding this comment.
Yes, you are right! I"m removing all those files
| private fun init(context: Context, attrs: AttributeSet?) { | ||
|
|
||
| val ta: TypedArray = context.obtainStyledAttributes(attrs, R.styleable.BottomNavigationBar) | ||
| val resourceId = ta.getResourceId(R.styleable.BottomNavigationBar_layoutResource, R.layout.layout_browser_bottom_navigation_bar) |
There was a problem hiding this comment.
attrs.xml -> to remove styleable BottomNavigationBar
Also bottom_navigation_bar_bg.xml, but I couldn't find where we were using that background in develop 🤷♀️
| private fun isBottomNavigationFeatureEnabled() = | ||
| variantManager.getVariant().hasFeature(VariantManager.VariantFeature.BottomBarNavigation) | ||
|
|
||
| private fun decorateWithFeatures() { |
There was a problem hiding this comment.
What about removing this method and calling directly from onActivityCreated to decorator.decorateWithFeatures() (including the renameing of decorator.decorate() to decorator.decorateWithFeatures()) ?
It's just to simplify method forwarding
There was a problem hiding this comment.
that's a good idea, no need to have calls to other methods if they are only called from one place
| configureLongClickOpensNewTabListenerWithToolbarOnly() | ||
| removeUnnecessaryLayoutBehaviour() | ||
| fun decorateToolbarMenus(viewState: BrowserViewState) { | ||
| decorator.updateToolbarActionsVisibility(viewState) |
There was a problem hiding this comment.
replace method call with updateToolbarActionsVisibility implementation? Also to simplify method forwarding for just one line.
|
@cmonfortep I've added the changes you requested. Thanks for the review! |
2 participants
Task/Issue URL: https://app.asana.com/0/72649045549333/1177859689086266
Tech Design URL:
CC:
Description:
The Bottom Navigation experiment is now concluded and we've agreed to remove it for now. This PR removes all the code from the experiment.
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template