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

Use global navigation action for browser fragment #4691

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

NotWoods
Copy link
Contributor

Global actions are meant to let multiple fragments navigate to the name destination. We can use the global browser fragment action for all browser fragment navigation and get rid of the complex switch case.

Builds towards separate custom tab fragment and PWA support.

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

@NotWoods NotWoods requested a review from a team as a code owner August 12, 2019 17:28
@codecov-io
Copy link

codecov-io commented Aug 12, 2019

Codecov Report

Merging #4691 into master will increase coverage by 0.09%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #4691      +/-   ##
===========================================
+ Coverage      6.69%   6.79%   +0.09%     
- Complexity      152     154       +2     
===========================================
  Files           203     204       +1     
  Lines          8494    8480      -14     
  Branches       1189    1189              
===========================================
+ Hits            569     576       +7     
+ Misses         7886    7865      -21     
  Partials         39      39
Impacted Files Coverage Δ Complexity Δ
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt 1.91% <0%> (+0.23%) 3 <0> (ø) ⬇️
...rc/main/java/org/mozilla/fenix/BrowserDirection.kt 100% <100%> (ø) 2 <2> (?)
app/src/main/java/org/mozilla/fenix/ext/Context.kt 16% <0%> (-4%) 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1328d68...9eb3a4d. Read the comment docs.

Copy link
Contributor

@colintheshots colintheshots left a comment

Choose a reason for hiding this comment

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

Thanks for the change. I'm fairly sure Proguard/R8 doesn't know what to do with a Nullable enum, so it will go unoptimized. This is a minor gripe, sure, but it would be best to use only int primitives.

app/src/main/java/org/mozilla/fenix/BrowserDirection.kt Outdated Show resolved Hide resolved
@colintheshots colintheshots merged commit 3acabeb into mozilla-mobile:master Aug 14, 2019
@ekager
Copy link
Contributor

ekager commented Aug 21, 2019

I'm going to revert this. Since all of our browser navgiations go through HomeActivity we need to be careful about changing things there. Every action in the nav graph has it's own popTo, anim, etc defined in the nav graph. By using global for every action to the browser, we are breaking those.

@NotWoods
Copy link
Contributor Author

Sounds good, I'll explore other options for custom tabs fragment.

@NotWoods NotWoods deleted the browser-global-action branch January 25, 2020 19:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants