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

Create ToolbarPosition enum #12747

Merged
merged 1 commit into from Jul 23, 2020

Conversation

NotWoods
Copy link
Contributor

Using an enum makes code much easier to read because the top/bottom association is more obvious. This just adds a new getter to Settings without changing the backing field.

        // Before
        val isBottomToolbar = Settings.getInstance(context).shouldUseBottomToolbar

        layout.drop_down_triangle.isGone = isBottomToolbar
        layout.pop_up_triangle.isVisible = isBottomToolbar
        // After
        val toolbarPosition = context.settings().toolbarPosition

        layout.drop_down_triangle.isVisible = toolbarPosition == ToolbarPosition.TOP
        layout.pop_up_triangle.isVisible = toolbarPosition == ToolbarPosition.BOTTOM

@NotWoods NotWoods added the eng:health Improve code health label Jul 20, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2020

Codecov Report

Merging #12747 into master will increase coverage by 0.00%.
The diff coverage is 13.41%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #12747   +/-   ##
=========================================
  Coverage     27.26%   27.26%           
- Complexity      963      965    +2     
=========================================
  Files           401      402    +1     
  Lines         15974    15969    -5     
  Branches       2033     2032    -1     
=========================================
- Hits           4355     4354    -1     
+ Misses        11291    11286    -5     
- Partials        328      329    +1     
Impacted Files Coverage Δ Complexity Δ
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...main/java/org/mozilla/fenix/cfr/SearchWidgetCFR.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...lla/fenix/components/toolbar/BrowserToolbarView.kt 2.83% <0.00%> (+0.11%) 0.00 <0.00> (ø)
...enix/components/toolbar/TabCounterToolbarButton.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...c/main/java/org/mozilla/fenix/home/HomeFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...rg/mozilla/fenix/settings/CustomizationFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../src/main/java/org/mozilla/fenix/utils/Settings.kt 61.11% <0.00%> (-0.15%) 25.00 <0.00> (ø)
...la/fenix/components/metrics/GleanMetricsService.kt 12.70% <33.33%> (ø) 4.00 <0.00> (ø)
.../java/org/mozilla/fenix/browser/BrowserAnimator.kt 7.69% <40.00%> (ø) 0.00 <0.00> (ø)
...ix/trackingprotection/TrackingProtectionOverlay.kt 77.55% <50.00%> (-0.89%) 10.00 <0.00> (ø)
... and 4 more

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 c2d940c...cdd1746. Read the comment docs.

Copy link
Contributor

@BranescuMihai BranescuMihai left a comment

Choose a reason for hiding this comment

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

I really like this cleanup initiative!

I would recommend creating tickets for any cleanup PRs though, as every time we touch code besides tests, there's a change we introduce regressions, so they should be tested.

@NotWoods NotWoods merged commit 8f5a377 into mozilla-mobile:master Jul 23, 2020
@NotWoods NotWoods deleted the toolbar-position-setting branch July 23, 2020 02:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
eng:health Improve code health
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants