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

For #17802: Match the design for three-dot menu navigation #17875

Merged
merged 7 commits into from
Mar 4, 2021

Conversation

eliserichards
Copy link
Contributor

@eliserichards eliserichards commented Feb 5, 2021

For #17802
Wait for #17838 to be merged (✅ merged)

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.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@eliserichards eliserichards added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Feb 5, 2021
@eliserichards eliserichards requested review from a team as code owners February 5, 2021 22:01
@mergify
Copy link
Contributor

mergify bot commented Feb 12, 2021

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

@eliserichards eliserichards changed the title [WIP] For #17802: change three-dot menu navigation For #17802: change three-dot menu navigation Feb 12, 2021
@eliserichards eliserichards added needs:review PRs that need to be reviewed and removed pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on labels Feb 12, 2021
@codecov-io
Copy link

codecov-io commented Feb 12, 2021

Codecov Report

Merging #17875 (c52a62d) into master (8b7279e) will decrease coverage by 0.15%.
The diff coverage is 2.56%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #17875      +/-   ##
============================================
- Coverage     33.00%   32.84%   -0.16%     
+ Complexity     1416     1383      -33     
============================================
  Files           461      461              
  Lines         19472    19430      -42     
  Branches       2988     2719     -269     
============================================
- Hits           6426     6382      -44     
- Misses        12391    12512     +121     
+ Partials        655      536     -119     
Impacted Files Coverage Δ Complexity Δ
...lla/fenix/components/toolbar/DefaultToolbarMenu.kt 3.84% <2.56%> (+0.52%) 2.00 <0.00> (ø)
...fenix/trackingprotection/TrackingProtectionMode.kt 0.00% <0.00%> (-81.25%) 0.00% <0.00%> (-2.00%)
...pp/src/main/java/org/mozilla/fenix/ext/Activity.kt 20.00% <0.00%> (-70.00%) 0.00% <0.00%> (ø%)
.../main/java/org/mozilla/fenix/theme/ThemeManager.kt 9.75% <0.00%> (-31.71%) 0.00% <0.00%> (-5.00%)
...ain/java/org/mozilla/fenix/perf/StartupTimeline.kt 55.55% <0.00%> (-27.78%) 2.00% <0.00%> (-2.00%)
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt 6.39% <0.00%> (-17.67%) 14.00% <0.00%> (-14.00%)
...rc/main/java/org/mozilla/fenix/perf/Performance.kt 10.00% <0.00%> (-10.00%) 2.00% <0.00%> (-2.00%)
...g/mozilla/fenix/downloads/DynamicDownloadDialog.kt 0.00% <0.00%> (-6.78%) 0.00% <0.00%> (ø%)
...nix/components/toolbar/BrowserToolbarController.kt 89.65% <0.00%> (-1.73%) 0.00% <0.00%> (ø%)
...e/intent/FennecBookmarkShortcutsIntentProcessor.kt 91.66% <0.00%> (-1.67%) 5.00% <0.00%> (ø%)
... and 74 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 8b7279e...5b05a2d. Read the comment docs.

@eliserichards eliserichards requested review from kglazko and Mugurell and removed request for a team February 12, 2021 17:54
Copy link
Member

@gabrielluong gabrielluong left a comment

Choose a reason for hiding this comment

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

It was a bit hard for me to tell what the changes were about based on the issue name. Could we also reword it to something like: Move the menu navigation toolbar based on the toolbar orientation

@eliserichards
Copy link
Contributor Author

@gabrielluong the acceptance criteria in the issue is pretty extensive, you can see what changes were made there

@gabrielluong
Copy link
Member

@gabrielluong the acceptance criteria in the issue is pretty extensive, you can see what changes were made there

I can only note 2 particular things that the PR does from looking at the code:

  • Moving the menu navigation toolbar based on the toolbar orientation
  • Removing the bookmark from the menu navigation toolbar

Since the latter is rather minor, I think the suggested naming of the issue of just addressing the first issue still makes the primary headline. We can add the latter point into the commit description as well or have it as a separate commit. I think I am still looking for the original requested changes?

Copy link
Member

@gabrielluong gabrielluong left a comment

Choose a reason for hiding this comment

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

I think I am still waiting for the changes on the original feedback?

@eliserichards eliserichards changed the title For #17802: change three-dot menu navigation For #17802: match design for three-dot menu nav Feb 12, 2021
@eliserichards
Copy link
Contributor Author

I think naming it as matching the design will point them to the issue so they can see the fine details. This naming encompasses all of the changes, instead of just mentioning one change

@eliserichards eliserichards changed the title For #17802: match design for three-dot menu nav For #17802: match design for three-dot menu navigation Feb 12, 2021
Copy link
Member

@gabrielluong gabrielluong 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, thank you for the changes!

@gabrielluong gabrielluong changed the title For #17802: match design for three-dot menu navigation For #17802: Match the design for three-dot menu navigation Feb 12, 2021
@gabrielluong gabrielluong added pr:approved PR that has been approved pr:needs-landing PRs that are ready to land [Will be merged by Mergify] and removed needs:review PRs that need to be reviewed labels Feb 12, 2021
@gabrielluong gabrielluong self-assigned this Feb 12, 2021
@gabrielluong gabrielluong reopened this Feb 13, 2021
@mergify
Copy link
Contributor

mergify bot commented Feb 20, 2021

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

@eliserichards eliserichards merged commit 42cc4cb into mozilla-mobile:master Mar 4, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this pull request Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:approved PR that has been approved pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants