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

For #17771: three-dot menu reorder #17838

Merged

Conversation

eliserichards
Copy link
Contributor

@eliserichards eliserichards commented Feb 4, 2021

Meta: #17796
Fixes #17771

Icons will be removed in #17872 (along with the erroneous add-ons menu that is added on the AC side in the WebExtensionBrowserMenuBuilder).
image

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 requested review from a team as code owners February 4, 2021 22:08
@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 4, 2021
@sv-ohorvath
Copy link
Contributor

Please rebase, a blocking UI test was disabled: f0b7b3b

@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 5, 2021
@eliserichards eliserichards changed the title [WIP] For #17771: three-dot menu reorder For #17771: three-dot menu reorder Feb 5, 2021
@codecov-io
Copy link

Codecov Report

Merging #17838 (c0f939f) into master (f5b068a) will decrease coverage by 0.61%.
The diff coverage is 20.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #17838      +/-   ##
============================================
- Coverage     33.29%   32.67%   -0.62%     
+ Complexity     1347     1346       -1     
============================================
  Files           457      457              
  Lines         19261    19382     +121     
  Branches       2700     2708       +8     
============================================
- Hits           6412     6334      -78     
- Misses        12315    12516     +201     
+ Partials        534      532       -2     
Impacted Files Coverage Δ Complexity Δ
...lla/fenix/components/toolbar/DefaultToolbarMenu.kt 3.60% <0.93%> (-39.13%) 2.00 <0.00> (-1.00)
...components/toolbar/BrowserToolbarMenuController.kt 72.67% <69.62%> (-1.45%) 0.00 <0.00> (ø)
...pp/src/main/java/org/mozilla/fenix/FeatureFlags.kt 100.00% <100.00%> (ø) 4.00 <0.00> (ø)
...rg/mozilla/fenix/components/toolbar/ToolbarMenu.kt 86.95% <100.00%> (-8.50%) 0.00 <0.00> (ø)

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 f5b068a...c0f939f. Read the comment docs.

onItemTapped.invoke(ToolbarMenu.Item.Settings)
}
private val newCoreMenuItems by lazy {
val newTabItem = BrowserMenuImageText(
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making the icon invisible, you could also use SimpleBrowserMenuItem here to remove the icon or do it as a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a followup filed for this - good to know which one we should use for that! I'll add it to the issue

@mergify
Copy link
Contributor

mergify bot commented Feb 9, 2021

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

}

@Test
fun handleToolbarBookmarkPressWithReaderModeActive() = runBlockingTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's been varied use of this in Fenix, but I was once told that naming tests with the back apostrophe and the IF THEN WHEN mindset. So fun Toolbar Bookmark Press With Readermode Inactive Results in Metrics Being Tracked where the individual can see what the test is supposed to verify from the title alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! I'll change this 👍

@kglazko
Copy link
Contributor

kglazko commented Feb 10, 2021

Also these lints should be removed [task 2021-02-05T23:23:46.731Z] /builds/worker/checkouts/src/app/src/androidTest/java/org/mozilla/fenix/ui/ReaderViewTest.kt:15:1: Unused import [task 2021-02-05T23:23:46.934Z] /builds/worker/checkouts/src/app/src/androidTest/java/org/mozilla/fenix/ui/TabbedBrowsingTest.kt:16:1: Unused import

@eliserichards eliserichards force-pushed the 17771-reorder-home-menu branch 2 times, most recently from 8da4e0f to bcfe1fa Compare February 11, 2021 22:22
@eliserichards eliserichards force-pushed the 17771-reorder-home-menu branch 2 times, most recently from 62e4c28 to 25f45bc Compare February 11, 2021 23:07
Copy link
Contributor

@kglazko kglazko left a comment

Choose a reason for hiding this comment

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

Looking good so far, if no further issues after the tests run, then approving. Let's file follow-up issues for each ignored test so that we can fix them in the near future and not lose track of them.

@eliserichards eliserichards reopened this Feb 12, 2021
@eliserichards eliserichards merged commit d0fd3e8 into mozilla-mobile:master Feb 12, 2021
@gabrielluong gabrielluong removed the needs:review PRs that need to be reviewed label Feb 12, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this pull request Aug 4, 2021
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.

[Feature] Three-dot menu item reordering
5 participants