Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Conversation

@mcarare
Copy link
Contributor

@mcarare mcarare commented Oct 4, 2019


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

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@mcarare
Copy link
Contributor Author

mcarare commented Oct 4, 2019

@pocmo This just fixes the shape.
There is still the same issue as for all icons in browser-toolbar, the ripple is bigger than the ones in home screen, and also they appear behind the url view.
I know you are working on #1514. Do you intend to fix these issues there or should I open a new issue waiting for your work there to be done? TY!

@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #4641 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4641      +/-   ##
============================================
+ Coverage     80.94%   81.06%   +0.11%     
+ Complexity     4075     3725     -350     
============================================
  Files           523      474      -49     
  Lines         17757    16437    -1320     
  Branches       2631     2479     -152     
============================================
- Hits          14373    13324    -1049     
+ Misses         2336     2143     -193     
+ Partials       1048      970      -78
Impacted Files Coverage Δ Complexity Δ
...a/components/browser/toolbar/display/MenuButton.kt 81.39% <100%> (ø) 14 <0> (ø) ⬇️
...ponents/service/experiments/ExperimentEvaluator.kt 85.57% <0%> (-0.97%) 57% <0%> (-1%)
...a/components/service/fxa/FxaDeviceConstellation.kt 86.2% <0%> (-0.24%) 13% <0%> (ø)
...onents/support/sync/telemetry/BaseGleanSyncPing.kt 100% <0%> (ø) 10% <0%> (-1%) ⬇️
...s/browser/icons/processor/AdaptiveIconProcessor.kt
...nts/lib/crash/service/GleanCrashReporterService.kt
...nts/browser/icons/processor/MemoryIconProcessor.kt
...nents/browser/icons/processor/DiskIconProcessor.kt
...a/mozilla/components/browser/icons/BrowserIcons.kt
.../components/browser/icons/extension/IconMessage.kt
... and 91 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 5bb5406...109fc3d. Read the comment docs.

Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

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

Thanks!

bors r+

bors bot pushed a commit that referenced this pull request Oct 8, 2019
4641: For #4639 Change menu button ripple to circular (unbounded) r=pocmo a=mcarare



Co-authored-by: mcarare <mihai.carare.dev@gmail.com>
@pocmo
Copy link
Contributor

pocmo commented Oct 8, 2019

I know you are working on #1514. Do you intend to fix these issues there or should I open a new issue waiting for your work there to be done? TY!

Yeah, I can look at this as part of #1514. I saw that sblatz also filed #4673. I'll keep that around as reminder.

@bors
Copy link

bors bot commented Oct 8, 2019

Build succeeded

  • complete-push

@bors bors bot merged commit 109fc3d into mozilla-mobile:master Oct 8, 2019
@mcarare mcarare deleted the 4639 branch December 17, 2019 08:25
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.

2 participants