Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Bug 1879370 - Add a ClippingBehavior supporting multiple toolbars #5988

Merged
merged 2 commits into from
Mar 14, 2024
Merged

Bug 1879370 - Add a ClippingBehavior supporting multiple toolbars #5988

merged 2 commits into from
Mar 14, 2024

Conversation

mavduevskiy
Copy link
Contributor

@mavduevskiy mavduevskiy commented Mar 12, 2024 •
edited by github-actions bot
Loading

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1879370

@github-actions github-actions bot added the work in progress Not ready to land yet. Work in progress (WIP). label Mar 12, 2024
@mavduevskiy mavduevskiy force-pushed the navbar_clipping branch 2 times, most recently from 5494464 to f0510fd Compare March 12, 2024 03:45
@mavduevskiy mavduevskiy marked this pull request as ready for review March 12, 2024 03:45
@github-actions github-actions bot added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Mar 12, 2024
@mavduevskiy mavduevskiy force-pushed the navbar_clipping branch 4 times, most recently from 074974b to 6f7540c Compare March 12, 2024 03:57
@mavduevskiy mavduevskiy requested a review from sarah541 March 12, 2024 04:02
rahulsainani
rahulsainani reviewed Mar 12, 2024
View reviewed changes
fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/RedesignToolbarFeature.kt Outdated
* The bottom toolbar can consist of a navigation bar,
* a combination of a navigation and address bar, or be absent.
*/
fun Context.getBottomToolbarHeight(): Int {
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Think we can move this and getTopToolbarHeight to Settings.kt

Also, thoughts on naming this tobottomBarsHeight since there can be two bars at the bottom and not just toolbar?

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 agree we could find a better place for the extension, but is Settings.kt a good fit? It doesn't have any extension functions.

It might be slightly confusing and lead readers to believe that there should be a different name for getting a single bottomBarHeight🤔 In context of toolbar translations, I believe it's better to think about toolbars as a top/bottom pair. From the Behaviour perspective, there are two ScrollableToolbar one at the top, one at the bottom, and sometimes they are there together. It doesn't really care about the contents, only about positioning.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Settings is injected with appContext so we have prefs and can access resources too. In the long run, each feature should have it's own prefs but since we use Settings.kt as the only pref service, think it makes sense to put it there.

That's a good point and perspective to look at the toolbars and is in sync with the rest of the implementation. Top/bottom pair 👍

@mavduevskiy mavduevskiy force-pushed the navbar_clipping branch 2 times, most recently from 0d11c18 to c02ebf6 Compare March 13, 2024 01:58
rahulsainani
rahulsainani approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@rahulsainani rahulsainani left a comment

Choose a reason for hiding this comment

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

Overall this is really good! 💯 Thanks for updating the docs!

A couple of comments about prefs and test contants!

rahulsainani
rahulsainani reviewed Mar 13, 2024
View reviewed changes
.../app/src/main/java/org/mozilla/fenix/components/toolbar/navbar/EngineViewClippingBehavior.kt
Comment on lines +80 to +82
is BrowserToolbar -> {
if (hasTopToolbar) {
recentTopToolbarTranslation = dependency.translationY
} else {
recentBottomToolbarTranslation = dependency.translationY
}
}
is ToolbarContainerView -> recentBottomToolbarTranslation = dependency.translationY
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking out loud, here we could try to use the interface ScrollableToolbar to get this information so we don't have to depend on these implementations and we can also move this to AC module in that case.

Seems like we only need to know the positioning of the ScrollableToolbar to act, so maybe a position() function in the interface that returns an enum could help here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an awesome idea! The future work should be exploring this idea in more detail✌️

@rocketsroger rocketsroger added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Mar 13, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 13, 2024

This pull request has conflicts when rebasing. Could you fix it @mavduevskiy? 🙏

@mavduevskiy mavduevskiy force-pushed the navbar_clipping branch 4 times, most recently from dc3efa7 to d0841b2 Compare March 13, 2024 21:43
rocketsroger
rocketsroger approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@rocketsroger rocketsroger left a comment

Choose a reason for hiding this comment

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

Looks good. Confirmed working as advertised. 🚢

@mavduevskiy mavduevskiy added the 🛬 needs landing PRs that are ready to land label Mar 13, 2024
@rvandermeulen rvandermeulen merged commit a999cdf into mozilla-mobile:main Mar 14, 2024
22 of 25 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rahulsainani rahulsainani rahulsainani approved these changes

@rocketsroger rocketsroger rocketsroger approved these changes

@sarah541 sarah541 Awaiting requested review from sarah541

@gabrielluong gabrielluong Awaiting requested review from gabrielluong

Assignees

@gabrielluong gabrielluong

@rocketsroger rocketsroger

@sarah541 sarah541

@rahulsainani rahulsainani

Labels
approved PR that has been approved 🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mavduevskiy @gabrielluong @rocketsroger @rahulsainani @rvandermeulen @sarah541