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

For #2754 Add tab cards to share sheet #5493

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

colintheshots
Copy link
Contributor

@colintheshots colintheshots commented Sep 23, 2019

This is using the new UX, not the old version at the top of the ticket.

device-2019-09-23-151718
device-2019-09-23-151845

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
  • 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

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

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

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #5493 into master will decrease coverage by 0.03%.
The diff coverage is 5.4%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5493      +/-   ##
============================================
- Coverage     14.04%   14.01%   -0.04%     
  Complexity      311      311              
============================================
  Files           255      256       +1     
  Lines         10414    10445      +31     
  Branches       1507     1509       +2     
============================================
+ Hits           1463     1464       +1     
- Misses         8840     8870      +30     
  Partials        111      111
Impacted Files Coverage Δ Complexity Δ
...main/java/org/mozilla/fenix/share/ShareFragment.kt 1.06% <0%> (-0.05%) 0 <0> (ø)
...zilla/fenix/share/listadapters/ShareTabsAdapter.kt 0% <0%> (ø) 0 <0> (?)
...ain/java/org/mozilla/fenix/share/ShareCloseView.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...g/mozilla/fenix/library/history/HistoryFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...illa/fenix/library/bookmarks/BookmarkController.kt 100% <100%> (ø) 0 <0> (ø) ⬇️
...va/org/mozilla/fenix/onboarding/FenixOnboarding.kt 0% <0%> (ø) 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 1c9c531...d6db5eb. Read the comment docs.

@sblatz
Copy link
Contributor

sblatz commented Sep 24, 2019

@colintheshots Looks like this is failing on the ShareButtonAppearanceTest UI test.

@boek
Copy link
Contributor

boek commented Sep 24, 2019

PR has updated strings, wait until we cut the release branch to land.

Copy link
Contributor

@boek boek 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! Just needs a missing license

@@ -0,0 +1,48 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: license

ekager
ekager previously requested changes Sep 24, 2019
Copy link
Contributor

@ekager ekager left a comment

Choose a reason for hiding this comment

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

In the first screenshot you posted it looks like there's a white sliver of background now on the corner of the share sheet? Could you take a look and fix that if it's a visual regression?

@colintheshots
Copy link
Contributor Author

colintheshots commented Sep 24, 2019

@ekager There's no more white sliver now.
@sblatz The test is now fixed.
@boek This can wait to land then.
Thanks!

@sblatz
Copy link
Contributor

sblatz commented Sep 25, 2019

@colintheshots I believe you'll need to rebase master into this since we fixed a bad UI test that got merged into master.

@colintheshots
Copy link
Contributor Author

@ekager This needs re-review. The white next to the corners showing through has been fixed.

@sblatz sblatz dismissed ekager’s stale review September 26, 2019 21:25

Concern has been fixed

@sblatz sblatz merged commit de93b05 into mozilla-mobile:master Sep 26, 2019
@colintheshots colintheshots deleted the share_tabs branch September 28, 2019 23:33
sblatz pushed a commit to sblatz/fenix that referenced this pull request Sep 30, 2019
…5493)

* For mozilla-mobile#2754 Add tab cards to share sheet

* For mozilla-mobile#2754: Fix background near rounded corners and ShareButtonAppearanceTest

* Add license to share_tab_item
sblatz pushed a commit to sblatz/fenix that referenced this pull request Sep 30, 2019
…5493)

* For mozilla-mobile#2754 Add tab cards to share sheet

* For mozilla-mobile#2754: Fix background near rounded corners and ShareButtonAppearanceTest

* Add license to share_tab_item
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants