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

For #19916 - Add last viewed tab to home screen #19944

Merged
merged 1 commit into from
Jun 10, 2021
Merged

For #19916 - Add last viewed tab to home screen #19944

merged 1 commit into from
Jun 10, 2021

Conversation

gabrielluong
Copy link
Member

@gabrielluong gabrielluong commented Jun 10, 2021 •
edited by jonalmeida
Loading

Fixes #19916

Screen Shot 2021-06-10 at 10 49 38 AM

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

@gabrielluong gabrielluong requested review from a team as code owners June 10, 2021 15:06
@rocketsroger rocketsroger added needs:review PRs that need to be reviewed needs:UX-review labels Jun 10, 2021
jonalmeida
jonalmeida approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait for @rocketsroger 's review since mine is pretty biased. 😄

@gabrielluong @jonalmeida
For mozilla-mobile#19916 - Add last viewed tab to home screen
2b0dae5 
Co-authored-by: Jonathan Almeida <jalmeida@mozilla.com>
@gabrielluong
Copy link
Member Author

gabrielluong commented Jun 10, 2021 •
edited
Loading

Already demo with @topotropic to get her blessing. Won't block on the UX-review.

@gabrielluong gabrielluong added needs:review PRs that need to be reviewed and removed needs:review PRs that need to be reviewed needs:UX-review labels Jun 10, 2021
jonalmeida
jonalmeida reviewed Jun 10, 2021
View reviewed changes
app/src/main/java/org/mozilla/fenix/home/sessioncontrol/RecentTabsListFeature.kt
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package org.mozilla.fenix.home.sessioncontrol
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Probably meant for this (and it's test) to be in org.mozilla.fenix.home.recenttab.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do this as a fast follow to not lose the green CI

app/src/main/res/layout/recent_tabs_header.xml
xmlns:app="http://schemas.android.com/apk/res-auto"
android:layout_width="match_parent"
android:layout_height="56dp"
android:layout_marginTop="40dp">
Copy link
Contributor

@jonalmeida jonalmeida Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This looks wrong. We're adding an inconsistent amount of margin above this section.

(on the left the current nightly, on the right is this PR)

out

Copy link
Member Author

Choose a reason for hiding this comment

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

isn't the pager dots?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you can see in the left screenshot which "container" the top sites dots goes in. Also, removing this margin line takes away the weird space. I added 10dp in this screenshot:

We want to use something like 8/16/24dp.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still unsure about this. Let's discuss and follow up.

rocketsroger
rocketsroger approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@rocketsroger rocketsroger left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@gabrielluong gabrielluong merged commit 9d3cf79 into mozilla-mobile:master Jun 10, 2021
@gabrielluong gabrielluong deleted the 19916 branch June 10, 2021 20:23
@gabrielluong gabrielluong mentioned this pull request Jun 10, 2021
Closed
7 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonalmeida jonalmeida jonalmeida approved these changes

@rocketsroger rocketsroger rocketsroger approved these changes

@topotropic topotropic Awaiting requested review from topotropic

Assignees

@jonalmeida jonalmeida

@rocketsroger rocketsroger

Labels
needs:review PRs that need to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add last viewed tab to homescreen
3 participants
@gabrielluong @jonalmeida @rocketsroger