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

For #21236: Separate tabs with the same search term into a different section #21177

Merged
merged 3 commits into from Sep 17, 2021
Merged

For #21236: Separate tabs with the same search term into a different section #21177

merged 3 commits into from Sep 17, 2021

Conversation

rocketsroger
Copy link
Contributor

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

@rocketsroger rocketsroger added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Sep 7, 2021
@rocketsroger rocketsroger requested review from a team as code owners September 7, 2021 22:29
jonalmeida
jonalmeida reviewed Sep 8, 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.

I did a pass on the changes to keep a note for ourselves. We can work on them in time if we want to focus on the next bits of things to do.

app/src/main/java/org/mozilla/fenix/tabstray/browser/TabGroupAdapter.kt Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/tabstray/browser/TabGroupAdapter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/tabstray/browser/TabGroupAdapter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/tabstray/browser/TabGroupAdapter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/mozilla/fenix/tabstray/browser/TabGroupController.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/tab_group_footer_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/tab_group_item.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/tab_group_list_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/tab_group_item.xml Show resolved Hide resolved
@jonalmeida jonalmeida marked this pull request as draft September 8, 2021 23:02
@jonalmeida
Copy link
Contributor

(converted PR to a draft since it's still a WIP)

@rocketsroger rocketsroger force-pushed the tabs_group branch 2 times, most recently from 624ba7f to 55e6bc7 Compare September 9, 2021 17:03
@jonalmeida
Copy link
Contributor

jonalmeida commented Sep 9, 2021 •
edited

Before we land this, we need to figure out the scrollToPosition as well.

I left a comment on #21218 (comment) but we'll have to fix it here.

There's also the oddity that the scroll to position is passed down through the viewholders and we probably have to refactor that part to when we're fixing this.

@rocketsroger rocketsroger force-pushed the tabs_group branch 7 times, most recently from 447a1e1 to cab9561 Compare September 10, 2021 17:44
@rocketsroger rocketsroger changed the title [WIP] Search term groups for Tabs Tray For #21236: Separate tabs with the same search term into a different section Sep 10, 2021
@rocketsroger rocketsroger 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 Sep 16, 2021
@jonalmeida jonalmeida marked this pull request as ready for review September 16, 2021 23:41
@rocketsroger rocketsroger reopened this Sep 17, 2021
jonalmeida
jonalmeida approved these changes Sep 17, 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.

Let's land this and see it in nightly. 🎉

As a follow-up we should still add a Secret Settings pref.

@jonalmeida jonalmeida added pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] and removed needs:review PRs that need to be reviewed labels Sep 17, 2021
@mergify mergify bot merged commit fc18fd2 into mozilla-mobile:main Sep 17, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this pull request Sep 17, 2021
@rocketsroger @jonalmeida @pkirakosyan
For mozilla-mobile#21236: Separate tabs with the same search term int…
7b321a0 
…o a different section (mozilla-mobile#21177)

* For mozilla-mobile#21236: Separate tabs with the same search term into a different section in tabs tray

* Issue mozilla-mobile#21236: Scroll to selected tab + various tab fixes for groupings

* Issue mozilla-mobile#21236: Fix failing test

Co-authored-by: Jonathan Almeida <jalmeida@mozilla.com>
@rocketsroger rocketsroger deleted the tabs_group branch September 17, 2021 12:47
@rocketsroger rocketsroger added this to the 94 milestone Oct 5, 2021
@jonalmeida jonalmeida mentioned this pull request Nov 2, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MozillaNoah MozillaNoah MozillaNoah left review comments

@jonalmeida jonalmeida jonalmeida approved these changes

Assignees
No one assigned
Labels
pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify]
Projects
None yet
Milestone
94
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rocketsroger @jonalmeida @MozillaNoah