Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

For #7075: Expand toolbar on navigation or load request #7088

Closed
wants to merge 1 commit into from
Closed

For #7075: Expand toolbar on navigation or load request #7088

wants to merge 1 commit into from

Conversation

ryg-git
Copy link
Contributor

@ryg-git ryg-git commented May 23, 2022

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

May fix #7075

issue-7075

issue-7075.mp4

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to build-focus-debug or build-klar-debug for changes targeting Klar,
  3. click View task in Taskcluster,
  4. click the Artifacts row,
  5. click to download any of the apks listed here which use an appropriate name for each CPU architecture.

@firefoxci-taskcluster
Copy link

No Taskcluster jobs started for this pull request 
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@ryg-git ryg-git mentioned this pull request May 23, 2022
Closed
Mugurell
Mugurell reviewed May 23, 2022
View reviewed changes
app/src/main/java/org/mozilla/focus/browser/integration/BrowserToolbarIntegration.kt Outdated
@@ -275,6 +277,23 @@ class BrowserToolbarIntegration(
}
}

@VisibleForTesting
Copy link
Contributor

Choose a reason for hiding this comment

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

This annotation allows relaxing the visibility specifically for tests.
Will you add also add the tests for this method or can the annotation be removed and the method be marked as private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I will remove the annotation

@ryg-git ryg-git marked this pull request as ready for review May 23, 2022 16:31
Mugurell
Mugurell reviewed May 24, 2022
View reviewed changes
Copy link
Contributor

@Mugurell Mugurell left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!
Great that you also included the small video to showcase the result!

Can you please squash your commits to just one and update the commit message and PR title to include a reference to the ticket this patch intends to fix, making it easier to find it later?
As described here - https://github.com/mozilla-mobile/shared-docs/blob/main/android/CONTRIBUTING_code.md#creating-a-pull-request The resulting commit title would have the form

For #7075: Expand toolbar on navigation or load request

@ryg-git
fix mozilla-mobile#7075: make toolbar visible on navigation or load r…
42144fb 
…equest changes

added `expandToolbarOnNavigation`method which listens to changes in `tab.content.url` and `tab.content.loadRequest` to expand the toolbar and show loading progress.

closes mozilla-mobile#7075
@ryg-git ryg-git changed the title expand toolbar on navigation or load request For #7075: Expand toolbar on navigation or load request May 24, 2022
Mugurell
Mugurell approved these changes May 25, 2022
View reviewed changes
Copy link
Contributor

@Mugurell Mugurell left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@Mugurell Mugurell mentioned this pull request May 25, 2022
Merged
3 tasks
@Mugurell
Copy link
Contributor

@ryg-git There were some formatting issues causing the linting tasks to fail which I've addressed and will land your PR in #7111.
It's a bit of a more cumbersome process but you are still the author of the commit, thank you for that!

@ryg-git ryg-git closed this May 26, 2022
@ryg-git ryg-git deleted the issue-7075 branch May 26, 2022 01:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Mugurell Mugurell Mugurell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

show toolbar with loading progress when toolbar is hidden and url is tapped
2 participants
@ryg-git @Mugurell