-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #5694 & #6054: Allows users to change toolbar position #6608
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6608 +/- ##
===========================================
- Coverage 17.33% 17.24% -0.1%
- Complexity 390 391 +1
===========================================
Files 280 281 +1
Lines 11173 11274 +101
Branches 1567 1592 +25
===========================================
+ Hits 1937 1944 +7
- Misses 9082 9175 +93
- Partials 154 155 +1
Continue to review full report at Codecov.
|
d1ea29e
to
8f7c4d3
Compare
1e00e65
to
82d19c5
Compare
d02a2c7
to
1fbc3c7
Compare
Request for data collection review formAll questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.
|
1fbc3c7
to
2abb766
Compare
2abb766
to
5f3d632
Compare
5f3d632
to
d27f135
Compare
*/ | ||
override fun onDependentViewChanged(parent: CoordinatorLayout, child: View, dependency: View): Boolean { | ||
val engineView = child.findViewInHierarchy { it is EngineView } as EngineView? | ||
engineView?.setVerticalClipping(dependency.height - dependency.translationY.toInt()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty odd to me. This setVerticalClipping
API ends up calling setFixedBottomOffset
in GeckoView, the function does just set a margin for position:fixed elements at bottom of viewport. As far as I can tell, we need at least one new API make the top toolbar dynamic properly, the API would be a function to set an offset of the root viewport (i.e. not only for position:fixed elements).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for the top toolbar this should not be called at all. And I think @sblatz decided to not use this behavior implementation for the top toolbar, right? Afaik this was never really used in Fenix.
d27f135
to
9ac3e83
Compare
Could we make sure we're dealing with the snackbar anchoring correctly in browser fragments? Maybe add a new function that gets the anchor by checking the status like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to hear from UX on the home toolbar position, but from an engineering perspective I think this is looking really good. There are a few things that can be cleaned up before we merge though.
app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/components/toolbar/EngineViewTopBehavior.kt
Outdated
Show resolved
Hide resolved
app/src/main/res/layout/component_browser_top_toolbar_fixed.xml
Outdated
Show resolved
Hide resolved
app/src/main/res/layout/component_browser_top_toolbar_fixed.xml
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/org/mozilla/fenix/ui/robots/NavigationToolbarRobot.kt
Outdated
Show resolved
Hide resolved
e4a09fc
to
df2eb66
Compare
df2eb66
to
5b24b8f
Compare
Data Review Form (to be filled by Data Stewards)
|
Pull Request checklist
After merge
To download an APK when reviewing a PR: