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.

Sign up for GitHub

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

Jump to bottom

Bookmark all tabs - Context menu #2599

Merged
merged 6 commits into from
Apr 14, 2024
Merged

Bookmark all tabs - Context menu #2599

merged 6 commits into from
Apr 14, 2024

Conversation

alessandroboron
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/0/1207032400501906/f
CC: @samsymons

Description:

Add the new “Bookmark All Tabs…” menu item to:

  1. Main application Menu -> Bookmarks menu.
  2. The more options menu, the menu shown from the “…” button.
  3. The contextual menu of a single Tab.

NOTE

  1. We’re not adding “Bookmark All Tabs…” menu item to the contextual menu of Pinned Tabs.

Figma Mockup

Steps to test this PR:

Scenario 1: "Bookmark All Tabs…” shows in main application menu 🟢

  1. Click on Bookmarks main application menu item.
    Expected Result: "Bookmarks All Tabs…” menu item should be visible at the second position from the top.

Scenario 2: "Bookmark All Tabs…” shows in more options menu 🟢

  1. Click on the ... button at the right-hand side of the address bar.
  2. Select Bookmarks and wait for submenu to appear.
    Expected Result: "Bookmarks All Tabs…” menu item should be visible at the second position from the top.

Scenario 3: "Bookmark All Tabs…” shows in the Tab contextual menu 🟢

  1. Open a Tab.
  2. Right-click on the Tab and wait for the contextual menu to appear.
    Expected Result: "Bookmarks All Tabs…” menu item should be visible at the sixth position from the top.

Scenario 4: "Bookmark All Tabs…” does not shows in Pinned Tab contextual menu 🟢

  1. Open a Tab.
  2. Pin the Tab.
  3. Right-click on the Tab and wait for the contextual menu to appear.
    Expected Result: "Bookmarks All Tabs…” menu item should be not visible.

Scenario 5: "Bookmark All Tabs…” should be enabled when at least two non pinned tabs with a loaded web page are open - Main Menu 🟢

  1. Ensure there are two Tabs with a web page loaded.
  2. Click on Bookmarks main application menu item.
    Expected Result: "Bookmarks All Tabs…” menu item should be enabled.

Scenario 6: "Bookmark All Tabs…” should be enabled when at least two non pinned tabs with a loaded web page are open - More Options Menu 🟢

  1. Ensure there are two Tabs with a web page loaded.
  2. Click on the ... button at the right-hand side of the address bar.
  3. Select Bookmarks menu item and wait for submenu to appear.
    Expected Result: "Bookmarks All Tabs…” menu item should be enabled.

Scenario 7: "Bookmark All Tabs…” should be enabled when at least two non pinned tabs with a loaded web page are open - Tab Contextual Menu 🟢

  1. Ensure there are two Tabs with a web page loaded.
  2. Right-click on the Tab and wait for the contextual menu to appear.
    Expected Result: "Bookmarks All Tabs…” menu item should be enabled.

Scenario 8: "Bookmark All Tabs…” should be disabled when two pinned tabs with a loaded web page are open - Main Menu 🟢

  1. Ensure there are only two Tabs with a web page loaded.
  2. Pin both tabs.
  3. Click on Bookmarks main application menu item.
    Expected Result: "Bookmarks All Tabs…” menu item should be disabled.

Scenario 9: "Bookmark All Tabs…” should be disabled when two pinned tabs with a loaded web page are open - More Options Menu 🟢

  1. Ensure there are only two Tabs with a web page loaded.
  2. Pin both tabs.
  3. Click on the ... button at the right-hand side of the address bar.
  4. Select Bookmarks menu item and wait for submenu to appear.
    Expected Result: "Bookmarks All Tabs…” menu item should be disabled.

Scenario 10: "Bookmark All Tabs…” should be disabled when two pinned tabs with a loaded web page are open - More Options Menu 🟢

  1. Ensure there are only three Tabs with a web page loaded.
  2. Pin the first two.
    3.Right-click on the non pinned Tab and wait for the contextual menu to appear.
    Expected Result: "Bookmarks All Tabs…” menu item should be disabled.

—

Internal references:

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

@github-actions GitHub Actions
Copy link
Contributor

🚫 The Asana task linked in the PR description is not added to macOS App Board project.

  1. Verify that the correct task is linked in the PR.
    • ⚠️ Please use the actual implementation task, rather than the Code Review subtask.
  2. Verify that the task is added to macOS App Board project.
  3. When ready, remove the bot: not in app board label to retrigger the check.

@github-actions github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 11, 2024
alessandroboron
alessandroboron commented Apr 11, 2024
View reviewed changes
DuckDuckGo/Common/Localizables/UserText.swift
@@ -429,6 +429,7 @@ struct UserText {
static let editFavorite = NSLocalizedString("edit.favorite", value: "Edit Favorite", comment: "Header of the view that edits a favorite bookmark")
static let removeFromFavorites = NSLocalizedString("remove.from.favorites", value: "Remove from Favorites", comment: "Button for removing bookmarks from favorites")
static let bookmarkThisPage = NSLocalizedString("bookmark.this.page", value: "Bookmark This Page", comment: "Menu item for bookmarking current page")
static let bookmarkAllTabs = NSLocalizedString("bookmark.all.tabs", value: "Bookmark All Tabs…", comment: "Menu item for bookmarking all the open tabs")
Copy link
Contributor Author

@alessandroboron alessandroboron Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@SabrinaTardio I'm waiting for the Smartling invite. At the moment I added the string without the translation. I’ve created a subtask to add all the localized string. I’ll probably do in a different PR, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah you add it at the end the end the final one… normally is worth waiting for copy approval in ship review but in this case it’s pretty obvious I guess ahah so I would still ask for translations meanwhile

SabrinaTardio
SabrinaTardio approved these changes Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@SabrinaTardio SabrinaTardio left a comment

Choose a reason for hiding this comment

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

Look great!

DuckDuckGo/Common/Localizables/UserText.swift
@@ -429,6 +429,7 @@ struct UserText {
static let editFavorite = NSLocalizedString("edit.favorite", value: "Edit Favorite", comment: "Header of the view that edits a favorite bookmark")
static let removeFromFavorites = NSLocalizedString("remove.from.favorites", value: "Remove from Favorites", comment: "Button for removing bookmarks from favorites")
static let bookmarkThisPage = NSLocalizedString("bookmark.this.page", value: "Bookmark This Page", comment: "Menu item for bookmarking current page")
static let bookmarkAllTabs = NSLocalizedString("bookmark.all.tabs", value: "Bookmark All Tabs…", comment: "Menu item for bookmarking all the open tabs")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah you add it at the end the end the final one… normally is worth waiting for copy approval in ship review but in this case it’s pretty obvious I guess ahah so I would still ask for translations meanwhile

@alessandroboron alessandroboron merged commit dd83ac7 into alessandro/bookmark-all-tabs Apr 14, 2024
15 of 17 checks passed
@alessandroboron alessandroboron deleted the alessandro/bookmark-all-tabs-context-menu branch April 14, 2024 23:34
alessandroboron added a commit that referenced this pull request Apr 18, 2024
@alessandroboron
Bookmark all tabs - Context menu (#2599)
a950dbc 
Task/Issue URL: https://app.asana.com/0/0/1207032400501906/f

**Description**:
Add the new “Bookmark All Tabs…” menu item to:
1. Main application Menu -> Bookmarks menu.
2. The more options menu, the menu shown from the “…” button.
3. The contextual menu of a single Tab.
alessandroboron added a commit that referenced this pull request Apr 23, 2024
@alessandroboron
Bookmark all tabs - Context menu (#2599)
2790ce4 
Task/Issue URL: https://app.asana.com/0/0/1207032400501906/f

**Description**:
Add the new “Bookmark All Tabs…” menu item to:
1. Main application Menu -> Bookmarks menu.
2. The more options menu, the menu shown from the “…” button.
3. The contextual menu of a single Tab.
alessandroboron added a commit that referenced this pull request Apr 24, 2024
@alessandroboron
Bookmark all tabs - Context menu (#2599)
b8d96b4 
Task/Issue URL: https://app.asana.com/0/0/1207032400501906/f

**Description**:
Add the new “Bookmark All Tabs…” menu item to:
1. Main application Menu -> Bookmarks menu.
2. The more options menu, the menu shown from the “…” button.
3. The contextual menu of a single Tab.
alessandroboron added a commit that referenced this pull request Apr 24, 2024
@alessandroboron
Bookmark all tabs - Context menu (#2599)
b33437b 
Task/Issue URL: https://app.asana.com/0/0/1207032400501906/f

**Description**:
Add the new “Bookmark All Tabs…” menu item to:
1. Main application Menu -> Bookmarks menu.
2. The more options menu, the menu shown from the “…” button.
3. The contextual menu of a single Tab.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SabrinaTardio SabrinaTardio SabrinaTardio approved these changes

Assignees
No one assigned
Labels
bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alessandroboron @SabrinaTardio