Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to tab implementation #3660

Merged
merged 23 commits into from
Jan 17, 2025
Merged

Switch to tab implementation #3660

merged 23 commits into from
Jan 17, 2025

Conversation

mallexxx
Copy link
Collaborator

Task/Issue URL: https://app.asana.com/0/1202406491309510/1208236460435649/f
Tech Design URL: https://app.asana.com/0/72649045549333/1208270496320813/f

Description:

  • Switch to already open tab suggestions

Steps to test this PR:

  1. Open some tabs in different windows, pin some of them, include local files (pdf-s, html-s)
  2. Open new tab/new window/new fire window, start typing something from open tabs – validate different open tabs are suggested and opened when selected; New tab page should close when selected.

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@mallexxx mallexxx requested a review from tomasstrba December 11, 2024 11:41
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

@mallexxx, this is really great feature, I love it! I added couple of comments below and would like to resolve a question of suggesting tabs between regular windows and Fire Windows in Asana.

Also, suggesting a pinned tab didn't work for me. Am I missing something?

DuckDuckGo/Suggestions/Model/SuggestionContainer.swift Outdated Show resolved Hide resolved
@@ -787,6 +787,16 @@ extension TabCollectionViewModel {
return nil
}

func indexInAllTabs(where condition: (Tab) -> Bool) -> TabIndex? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍 Very useful

@mallexxx mallexxx changed the title Switch to tab implementation [WIP] Switch to tab implementation Dec 12, 2024
@mallexxx mallexxx marked this pull request as draft December 12, 2024 10:03
Copy link
Contributor

github-actions bot commented Dec 24, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against 038e9ba

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

@mallexxx , there are two issues which should be fixed before we move on:

  1. Activating existing pinned tab using the suggestion in case there is just one regular tab open, closes the whole window
  2. Regular pinned tabs are being suggested in Fire Window. Since they have a different data store, they probably shouldn't be there

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

The PR is still a draft PR and marked as WIP, but I suppose it's ready for a final review

I tested scenarios I raised before and everything works as expected 👍 LGTM!

In case there already are two tabs with the same URL, we are suggesting the older one. I don't have a strong opinion, but as a user I'd prefer to have a tab there which was used most recently. This is definitely not a blocker.

@mallexxx mallexxx marked this pull request as ready for review January 17, 2025 11:54
@mallexxx mallexxx changed the title [WIP] Switch to tab implementation Switch to tab implementation Jan 17, 2025
@mallexxx mallexxx merged commit f105100 into main Jan 17, 2025
24 checks passed
@mallexxx mallexxx deleted the alex/tab-search branch January 17, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants