Show dialog to bookmark all tabs #2621

alessandroboron · 2024-04-15T05:20:13Z

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

Description:

Show the dialog to bookmark all tabs when clicking on the “Bookmark All Tabs…” context menu items.
Fix a bug when adding a new folder multiple times that caused the folder picker not to be updated and the folder name text field to be prefilled with the previously created name.

Figma Mockup

Screenshots

Steps to test this PR:

Scenario 1: Shows "Bookmark All Tabs…” dialog view - Main Application Menu 🟢

Ensure there are at least two unpinned Tabs with a web page loaded.
Click on Bookmarks main application menu item.
Click on Bookmark All Tabs… menu item.
Expected Result: The Bookmark All Tabs dialog should appear on screen.

Scenario 2: Shows "Bookmark All Tabs…” dialog view - More Options Menu 🟢

Ensure there are at least two unpinned Tabs with a web page loaded.
Click on the ... button at the right-hand side of the address bar.
Click on Bookmark All Tabs… menu item.
Expected Result: The Bookmark All Tabs dialog should appear on screen.

Scenario 3: Shows "Bookmark All Tabs…” dialog view - Tab Contextual Menu 🟢

Ensure there are at least two unpinned Tabs with a web page loaded.
Right-click on the Tab and wait for the contextual menu to appear.
Click on Bookmark All Tabs… menu item.
Expected Result: The Bookmark All Tabs dialog should appear on screen.

Scenario 4: "Bookmark All Tabs…” dialog view shows right title 🟢

Open 5 (you can choose any arbitrary number) tabs and load a web page for each tab.
Show Bookmark All Tabs… dialog view.
Expected Result: The dialog title should reflect the number of open tabs as per design. In the case there are 5 tabs with a web page loaded the title should be “Bookmark 5 Open Tabs”.

Scenario 5: "Bookmark All Tabs…” dialog view shows right folder name as suggestion 🟢

Open 5 (you can choose any arbitrary number) tabs and load a web page for each tab.
Show Bookmark All Tabs… dialog view.
Expected Result: The folder name should reflect Today’s date and the number of open tabs as per design. In the case there are 5 tabs with a web page loaded the title should be “yyyy-mm—dd - 5 Tabs”.
—

Internal references:

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

… consistency

…lder multiple times in the same session

github-actions · 2024-04-15T05:20:35Z

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

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

github-actions · 2024-04-15T05:22:07Z

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

Generated by 🚫 dangerJS against 7bc0262

alessandroboron · 2024-04-15T05:23:09Z

DuckDuckGo/Bookmarks/View/Dialog/BookmarkAllTabsDialogView.swift

+import SwiftUIExtensions
+
+struct BookmarkAllTabsDialogView: ModalView {
+    @ObservedObject private var viewModel: BookmarkAllTabsDialogCoordinatorViewModel<BookmarkAllTabsDialogViewModel, AddEditBookmarkFolderDialogViewModel>


Used same approach of AddEditBookmarkDialogView.

alessandroboron · 2024-04-15T05:25:28Z

DuckDuckGo/Bookmarks/View/Dialog/BookmarkDialogContainerView.swift

@@ -42,9 +42,13 @@ struct BookmarkDialogContainerView<Content: View, Buttons: View>: View {
                Text(title)
                    .foregroundColor(.primary)
                    .fontWeight(.semibold)
+                    .padding(.top, 20)


As BookmarkAllTabs dialog can have an educational message on multiple lines for long language I removed the fix height from the bookmark dialogs, moved the padding in here and let the subviews dictate the height. AddEditBookmarks still render properly.

alessandroboron · 2024-04-15T05:31:48Z

DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkFolderDialogViewModel.swift

@@ -42,8 +42,9 @@ final class AddEditBookmarkFolderDialogViewModel: BookmarkFolderDialogEditing {

    @Published var folderName: String
    @Published var selectedFolder: BookmarkFolder?
+    @Published private(set) var folders: [FolderViewModel]


Soooo, I found an issue when we present the Add folder dialog from AddEditBookmark and BookmarkAllTabs. Unfortunately this was missed during the project to address bookmark feedback (I’m pretty sure tho we would have tested this scenario, so I’m a bit 🙈) .

The issue happens when we add/edit a bookmark or bookmark all tabs, press the icon to add a folder, save and press again the icon to add a folder. The add folder dialog view shows the name of the previously saved folder rather than being empty and the picker selector is not updated.

I’ve fixed this and updated tests. I actually wrote an integration test.

alessandroboron · 2024-04-15T05:32:13Z

DuckDuckGo/Bookmarks/ViewModel/BookmarkAllTabsDialogCoordinatorViewModel.swift

+import SwiftUI
+import Combine
+
+final class BookmarkAllTabsDialogCoordinatorViewModel<BookmarkViewModel: BookmarkAllTabsDialogEditing, AddFolderViewModel: BookmarkFolderDialogEditing>: ObservableObject {


Same fashion on AddEditBookmarkDialogView

alessandroboron · 2024-04-15T05:32:41Z

DuckDuckGo/Localizable.xcstrings

@@ -7567,55 +7567,55 @@
      "localizations" : {
        "de" : {
          "stringUnit" : {
-            "state" : "translated",


Will open a PR soon for the localization of these strings

alessandroboron · 2024-04-15T05:33:43Z

LocalPackages/SwiftUIExtensions/Sources/SwiftUIExtensions/MultilineTextHeightFixer.swift

@@ -39,7 +39,7 @@ private struct MultilineTextHeightFixer: ViewModifier {
    }
 }

-extension View {
+public extension View {


I moved this ViewModifier which is helpful for multiline text from NetworkProtection to SwiftUIExtensions. I needed to render my text on multiple lines when I tested for long text. It wouldn’t work otherwise.

SabrinaTardio

Looks great and works as expected.
There are a couple of things to consider as concern copy! once we sort them we can merge (or if it is blocking you can merge it but we need to address thme before merging to main)

SabrinaTardio · 2024-04-16T08:42:35Z

DuckDuckGo/Bookmarks/Model/BookmarkManager.swift

@@ -348,6 +348,11 @@ final class LocalBookmarkManager: BookmarkManager {
        return results
    }

+    func bookmarkAll(websitesInfo: [WebsiteInfo], withinParentFolder parent: ParentFolderType) {
+        // TODO: https://app.asana.com/0/0/1207032959154802/f


I guess technically you can remove this? it does its duty

I put had a TODO because I still have to call sync, reload the bookmarks and write tests so I won’t forget it.

SabrinaTardio · 2024-04-16T08:57:15Z

DuckDuckGo/Bookmarks/View/Dialog/BookmarkAllTabsDialogView.swift

+#Preview("Bookmark All Tabs - Light") {
+    let parentFolder = BookmarkFolder(id: "7", title: "DuckDuckGo")
+    let bookmark = Bookmark(id: "1", url: "www.duckduckgo.com", title: "DuckDuckGo", isFavorite: true, parentFolderUUID: "7")
+    let bookmarkManager = LocalBookmarkManager(bookmarkStore: BookmarkStoreMock(bookmarks: [bookmark, parentFolder]))


Some failure here

It happens sometimes, I noticed that closing and re-opening Xcode fixes it 🙃

SabrinaTardio · 2024-04-16T09:33:29Z

DuckDuckGo/Common/Localizables/UserText.swift

@@ -1077,15 +1077,24 @@ struct UserText {
                static let editBookmark = NSLocalizedString("bookmarks.dialog.title.edit", value: "Edit Bookmark", comment: "Bookmark edit dialog title")
                static let addFolder = NSLocalizedString("bookmarks.dialog.folder.title.add", value: "Add Folder", comment: "Bookmark folder creation dialog title")
                static let editFolder = NSLocalizedString("bookmarks.dialog.folder.title.edit", value: "Edit Folder", comment: "Bookmark folder edit dialog title")
+                static let bookmarkOpenTabs = NSLocalizedString("bookmarks.dialog.allTabs.title.add", value: "Bookmark %1$d Open Tabs", comment: "Title of dialog to bookmark all open tabs. E.g. 'Bookmark 42 Open Tabs`")


I think there is an issue here since we do not support pluralisation we can really use "Bookmark %1$d Open Tabs”
We have to move towards something like “Bookmarks All Open Tabs (n)”
you should raise it with someone from copy

Ah very good catch with the plurals. I will speak with Product/Copy

SabrinaTardio · 2024-04-16T10:47:35Z

DuckDuckGo/Common/Localizables/UserText.swift

+                static let folderName = NSLocalizedString("bookmarks.dialog.field.folderName", value: "Folder Name", comment: "Folder name field label for Bookmarks folder")
+            }
+            enum Value {
+                static let folderName = NSLocalizedString("bookmarks.dialog.field.folderName", value: "%1$@ - %2$d Tabs", comment: "The suggested name of the folder that will contain the bookmark tabs. Eg. 2024-02-12 - 50 Tabs")


Same here at the very list we can ask the translator to deal with it using Tabs (n) for languages in which we have different plural forms

Ah very good catch with the plurals. I will speak with Product/Copy

alessandroboron · 2024-04-17T06:18:45Z

Looks great and works as expected. There are a couple of things to consider as concern copy! once we sort them we can merge (or if it is blocking you can merge it but we need to address thme before merging to main)

Thanks @SabrinaTardio!

Actually I was thinking we would never be in a scenario where we have

Bookmark 1 Tabs e 2024-04-17 - 1 Tabs because we need to have at least 2 open tabs to bookmark all tabs. Hence it will always be plural and not singular/plural depending if it’s 1 or n tabs.

Task/Issue URL: https://app.asana.com/0/0/1207032400501907/f **Description**: - Show the dialog to bookmark all tabs when clicking on the “Bookmark All Tabs…” context menu items. - Fix a bug when adding a new folder multiple times that caused the folder picker not to be updated and the folder name text field to be prefilled with the previously created name.

alessandroboron added 13 commits April 15, 2024 12:34

Add Copy for BookmarkAllTabs UI

a49a6a4

Present dialog to bookmark all tabs

24c6f2f

Refactor BookmarkAllTabs view and view logic

07fbd4e

Fix layout to support multiple line messages

05650c8

Add BookmarkFolderStore to BookmarkAllTabsViewModel

36a577e

Add bookmark all tabs skeleton for bookmarks manager and store

1ff7273

Add mock for BookmarkFolderStore

8adbcba

Add unit tests for BookmarkAllTabsViewModel

f5f3de8

Rename BookmarkAllTabsViewModel to BookmarkAllTabsDialogViewModel for…

fbce7ba

… consistency

Add tests for BookmarkAllTabsDialogCoordinatorViewModel

fa26092

Fix an issue that cause AddFolder view not to update when adding a fo…

976979d

…lder multiple times in the same session

Add tests for bookmark folders store

0e241ae

Fix message font

2d175c4

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 15, 2024

alessandroboron commented Apr 15, 2024

View reviewed changes

alessandroboron requested a review from SabrinaTardio April 15, 2024 05:34

SabrinaTardio reviewed Apr 16, 2024

View reviewed changes

alessandroboron added 2 commits April 17, 2024 15:23

Fix failing tests due to changes

9d2cf2e

Fix Preview as Mock are defined only for DEBUG config

7bc0262

alessandroboron requested a review from SabrinaTardio April 17, 2024 10:04

SabrinaTardio approved these changes Apr 17, 2024

View reviewed changes

alessandroboron merged commit 4b87398 into alessandro/bookmark-all-tabs Apr 18, 2024
15 of 17 checks passed

alessandroboron deleted the alessandro/bookmark-all-tabs-show-dialog branch April 18, 2024 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show dialog to bookmark all tabs #2621

Show dialog to bookmark all tabs #2621

alessandroboron commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 15, 2024 •

edited

Loading

alessandroboron Apr 15, 2024

alessandroboron Apr 15, 2024

alessandroboron Apr 15, 2024

alessandroboron Apr 15, 2024

alessandroboron Apr 15, 2024

alessandroboron Apr 15, 2024

SabrinaTardio left a comment

SabrinaTardio Apr 16, 2024

alessandroboron Apr 17, 2024

SabrinaTardio Apr 16, 2024

alessandroboron Apr 17, 2024

SabrinaTardio Apr 16, 2024

alessandroboron Apr 17, 2024

SabrinaTardio Apr 16, 2024

alessandroboron Apr 17, 2024

alessandroboron commented Apr 17, 2024

Show dialog to bookmark all tabs #2621

Show dialog to bookmark all tabs #2621

Conversation

alessandroboron commented Apr 15, 2024

Internal references:

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SabrinaTardio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandroboron commented Apr 17, 2024

github-actions bot commented Apr 15, 2024 •

edited

Loading