Skip to content

Commit

Permalink
Make it more obvious how to add an item in Autofill popover (#2040)
Browse files Browse the repository at this point in the history 
Task/Issue URL: https://app.asana.com/0/1177771139624306/1203861164086351/f
Tech Design URL:
CC:

Description:
Updates the autofill popover to have a more obvious "+ Add New" button, replacing the old "+" button that users weren't noticing
  • Loading branch information
@amddg44
amddg44 authored Jan 31, 2024
1 parent ae2727c commit e26ec38
Show file tree
Hide file tree
Showing 9 changed files with 182 additions and 19 deletions.
48 changes: 48 additions & 0 deletions DuckDuckGo/Localizable.xcstrings
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6626,6 +6626,54 @@
}
}
},
"pm.add.card" : {
"comment" : "Add New Credit Card button",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Add Credit Card"
}
}
}
},
"pm.add.identity" : {
"comment" : "Add New Identity button",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Add Identity"
}
}
}
},
"pm.add.login" : {
"comment" : "Add New Login button",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Add Password"
}
}
}
},
"pm.add.new" : {
"comment" : "Add New item button",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "Add New"
}
}
}
},
"pm.added" : {
"comment" : "Label for login added data",
"extractionState" : "extracted_with_value",
Expand Down
4 changes: 4 additions & 0 deletions DuckDuckGo/SecureVault/Extensions/UserText+PasswordManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ extension UserText {
static let pmEmptyStateCardsTitle = NSLocalizedString("pm.empty.cards.title", value: "No Cards", comment: "Label for cards empty state title")
static let pmEmptyStateNotesTitle = NSLocalizedString("pm.empty.notes.title", value: "No Notes", comment: "Label for notes empty state title")

static let pmAddItem = NSLocalizedString("pm.add.new", value: "Add New", comment: "Add New item button")
static let pmAddCard = NSLocalizedString("pm.add.card", value: "Add Credit Card", comment: "Add New Credit Card button")
static let pmAddLogin = NSLocalizedString("pm.add.login", value: "Add Password", comment: "Add New Login button")
static let pmAddIdentity = NSLocalizedString("pm.add.identity", value: "Add Identity", comment: "Add New Identity button")
static let pmNewCard = NSLocalizedString("pm.new.card", value: "Credit Card", comment: "Label for new card title")
static let pmNewLogin = NSLocalizedString("pm.new.login", value: "Password", comment: "Label for new login title")
static let pmNewIdentity = NSLocalizedString("pm.new.identity", value: "Identity", comment: "Label for new identity title")
Expand Down
11 changes: 9 additions & 2 deletions DuckDuckGo/SecureVault/Model/PasswordManagementItemListModel.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,15 +275,18 @@ final class PasswordManagementItemListModel: ObservableObject {
@Published var canChangeCategory: Bool = true

private var onItemSelected: (_ old: SecureVaultItem?, _ new: SecureVaultItem?) -> Void
private var onAddItemSelected: (_ category: SecureVaultSorting.Category) -> Void
private let tld: TLD
private let urlMatcher: AutofillDomainNameUrlMatcher
private static let randomColorsCount = 15

init(passwordManagerCoordinator: PasswordManagerCoordinating,
onItemSelected: @escaping (_ old: SecureVaultItem?, _ new: SecureVaultItem?) -> Void,
urlMatcher: AutofillDomainNameUrlMatcher = AutofillDomainNameUrlMatcher(),
tld: TLD = ContentBlocking.shared.tld) {
tld: TLD = ContentBlocking.shared.tld,
onItemSelected: @escaping (_ old: SecureVaultItem?, _ new: SecureVaultItem?) -> Void,
onAddItemSelected: @escaping (_ category: SecureVaultSorting.Category) -> Void) {
self.onItemSelected = onItemSelected
self.onAddItemSelected = onAddItemSelected
self.passwordManagerCoordinator = passwordManagerCoordinator
self.urlMatcher = urlMatcher
self.tld = tld
Expand Down Expand Up @@ -479,4 +482,8 @@ final class PasswordManagementItemListModel: ObservableObject {
return tld.eTLDplus1(name) ?? title
}

func onAddItemClickedFor(_ category: SecureVaultSorting.Category) {
onAddItemSelected(category)
}

}
5 changes: 4 additions & 1 deletion DuckDuckGo/SecureVault/View/PasswordManagementCreditCardItemView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ struct PasswordManagementCreditCardItemView: View {
VStack(alignment: .leading, spacing: 0) {

HeaderView()
.padding(.top, 16)
.padding(.bottom, model.isInEditMode ? 20 : 30)

EditableCreditCardField(textFieldValue: $model.cardNumber, title: UserText.pmCardNumber)
Expand Down Expand Up @@ -94,10 +95,12 @@ struct PasswordManagementCreditCardItemView: View {
Spacer(minLength: 0)

Buttons()
.padding(.top, model.isInEditMode ? 12 : 10)
.padding(.bottom, model.isInEditMode ? 12 : 3)

}
.frame(maxWidth: .infinity, maxHeight: .infinity)
.padding()
.padding(.horizontal)

}
.padding(EdgeInsets(top: 10, leading: 0, bottom: 10, trailing: 10))
Expand Down
4 changes: 3 additions & 1 deletion DuckDuckGo/SecureVault/View/PasswordManagementIdentityItemView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ struct PasswordManagementIdentityItemView: View {
}

Buttons()
.padding()
.padding(.top, editMode ? 4 : 10)
.padding(.bottom, editMode ? 12 : 3)
.padding(.horizontal)

}

Expand Down
81 changes: 81 additions & 0 deletions DuckDuckGo/SecureVault/View/PasswordManagementItemList.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ struct PasswordManagementItemListView: View {
}
}
}

Spacer(minLength: 0)

Divider()

PasswordManagementAddButton()
.padding()

}
}

Expand Down Expand Up @@ -131,6 +139,7 @@ struct PasswordManagementItemListCategoryView: View {
PasswordManagementSortButton(imageName: "SortDescending")
}
}
.padding(.vertical, -4)

}
}
Expand Down Expand Up @@ -392,3 +401,75 @@ struct PasswordManagementSortButton: View {
}

}

private struct PasswordManagementAddButton: View {

@EnvironmentObject var model: PasswordManagementItemListModel

var body: some View {

switch model.sortDescriptor.category {
case .allItems:
ZStack {
// Setting Menu label to empty string and overlaying with this as Menu will not allow the image + text to be centered
Text(UserText.pmAddItem)

Menu {
createMenuItem(imageName: "LoginGlyph", text: UserText.pmNewLogin, category: .logins)
createMenuItem(imageName: "IdentityGlyph", text: UserText.pmNewIdentity, category: .identities)
createMenuItem(imageName: "CreditCardGlyph", text: UserText.pmNewCard, category: .cards)
} label: {
Text("")
}
.modifier(HideMenuIndicatorModifier())
}
.padding(.vertical, -4)
case .logins:
createButton(text: UserText.pmAddLogin, category: model.sortDescriptor.category)
case .identities:
createButton(text: UserText.pmAddIdentity, category: model.sortDescriptor.category)
case .cards:
createButton(text: UserText.pmAddCard, category: model.sortDescriptor.category)
}

}

private func createMenuItem(imageName: String, text: String, category: SecureVaultSorting.Category) -> some View {

Button {
model.onAddItemClickedFor(category)
} label: {
HStack {
Image(imageName)
Text(text)
}
}

}

private func createButton(text: String, category: SecureVaultSorting.Category) -> some View {

Button {
model.onAddItemClickedFor(category)
} label: {
Text(text)
.frame(maxWidth: .infinity)
.offset(y: 1)
}
.padding(.vertical, -4)

}
}

private struct HideMenuIndicatorModifier: ViewModifier {

func body(content: Content) -> some View {
if #available(macOS 12, *) {
content
.menuIndicator(.hidden)
} else {
content
}
}

}
4 changes: 3 additions & 1 deletion DuckDuckGo/SecureVault/View/PasswordManagementLoginItemView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ struct PasswordManagementLoginItemView: View {
}

Buttons()
.padding()
.padding(.top, editMode ? 12 : 10)
.padding(.bottom, editMode ? 12 : 3)
.padding(.horizontal)

}
}
Expand Down
16 changes: 14 additions & 2 deletions DuckDuckGo/SecureVault/View/PasswordManagementViewController.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -689,8 +689,9 @@ final class PasswordManagementViewController: NSViewController {
var passwordManagerSelectionCancellable: AnyCancellable?

// swiftlint:disable function_body_length
// swiftlint:disable:next cyclomatic_complexity
private func createListView() {
let listModel = PasswordManagementItemListModel(passwordManagerCoordinator: self.passwordManagerCoordinator) { [weak self] previousValue, newValue in
let listModel = PasswordManagementItemListModel(passwordManagerCoordinator: self.passwordManagerCoordinator, onItemSelected: { [weak self] previousValue, newValue in
guard let newValue = newValue,
let id = newValue.secureVaultID,
let window = self?.view.window else {
Expand Down Expand Up @@ -748,7 +749,18 @@ final class PasswordManagementViewController: NSViewController {
} else {
loadNewItemWithID()
}
}
}, onAddItemSelected: { [weak self] category in
switch category {
case .logins:
self?.createNewLogin()
case .identities:
self?.createNewIdentity()
case .cards:
self?.createNewCreditCard()
default:
break
}
})

self.listModel = listModel
self.listView = NSHostingView(rootView: PasswordManagementItemListView().environmentObject(listModel))
Expand Down
28 changes: 16 additions & 12 deletions UnitTests/SecureVault/PasswordManagementItemListModelTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ final class PasswordManagementItemListModelTests: XCTestCase {

func testWhenAccountIsSelectedThenModelReflectsThat() {
let accounts = (0 ..< 10).map { makeAccount(id: $0, domain: "domain\($0)") }
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)

model.update(items: accounts)
model.select(item: accounts[0])
Expand All @@ -47,7 +47,7 @@ final class PasswordManagementItemListModelTests: XCTestCase {
func testWhenFilterAppliedThenDisplayedAccountsOnlyContainFilteredMatches() {

let createdAccounts = (0 ..< 10).map { makeAccount(id: $0, domain: "domain\($0)") }
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)

model.update(items: createdAccounts)
model.filter = "domain5"
Expand All @@ -66,7 +66,7 @@ final class PasswordManagementItemListModelTests: XCTestCase {
}

func testWhenAccountIsSelectedThenCallbackReceivesOldAndNewVersion() {
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)
let account = makeAccount(id: 1)

model.update(items: [account])
Expand All @@ -77,20 +77,20 @@ final class PasswordManagementItemListModelTests: XCTestCase {
}

func testWhenGettingEmptyState_AndViewModelIsNewlyCreated_ThenEmptyStateIsNone() {
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)
XCTAssertEqual(model.emptyState, .none)
}

func testWhenGettingEmptyState_AndViewModelGetsGivenEmptyDataSet_ThenEmptyStateIsNoData() {
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)
model.update(items: [])

XCTAssertEqual(model.emptyState, .noData)
}

func testWhenGettingEmptyState_AndViewModelHasData_AndCategoryIsAllItems_AndViewModelIsFiltered_ThenEmptyStateIsNone() {
let accounts = (0 ..< 10).map { makeAccount(id: $0, domain: "domain\($0)") }
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)

model.update(items: accounts)
XCTAssertEqual(model.emptyState, .none)
Expand All @@ -104,7 +104,7 @@ final class PasswordManagementItemListModelTests: XCTestCase {

func testWhenGettingEmptyState_AndViewModelHasData_AndCategoryIsLogins_AndViewModelIsFiltered_ThenEmptyStateIsLogins() {
let accounts = (0 ..< 10).map { makeAccount(id: $0, domain: "domain\($0)") }
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)

model.update(items: accounts)
model.sortDescriptor.category = .logins
Expand All @@ -123,7 +123,7 @@ final class PasswordManagementItemListModelTests: XCTestCase {
let account3 = makeAccount(id: 3, domain: "otherdomain.com")
let accounts = [account1, account2, account3]

let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)

model.update(items: accounts)
model.selectLoginWithDomainOrFirst(domain: "dummy.com")
Expand All @@ -138,7 +138,7 @@ final class PasswordManagementItemListModelTests: XCTestCase {
let account3 = makeAccount(id: 3, domain: "otherdomain.com")
let accounts = [account1, account2, account3]

let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)

model.update(items: accounts)
model.selectLoginWithDomainOrFirst(domain: "example.com")
Expand All @@ -153,7 +153,7 @@ final class PasswordManagementItemListModelTests: XCTestCase {
let account3 = makeAccount(id: 3, domain: "www.example.com")
let accounts = [account1, account2, account3]

let model = PasswordManagementItemListModel(onItemSelected: onItemSelected)
let model = PasswordManagementItemListModel(onItemSelected: onItemSelected, onAddItemSelected: onAddItemSelected)

model.update(items: accounts)
model.selectLoginWithDomainOrFirst(domain: "sub.example.com")
Expand All @@ -180,6 +180,8 @@ final class PasswordManagementItemListModelTests: XCTestCase {
newSelection = new
}

func onAddItemSelected(category: SecureVaultSorting.Category) {}

private func accounts(from sections: [PasswordManagementListSection]) -> [SecureVaultModels.WebsiteAccount] {
var accounts = [SecureVaultModels.WebsiteAccount]()

Expand All @@ -201,9 +203,11 @@ final class PasswordManagementItemListModelTests: XCTestCase {

extension PasswordManagementItemListModel {

convenience init(onItemSelected: @escaping (_ old: SecureVaultItem?, _ new: SecureVaultItem?) -> Void) {
convenience init(onItemSelected: @escaping (_ old: SecureVaultItem?, _ new: SecureVaultItem?) -> Void,
onAddItemSelected: @escaping (_ category: SecureVaultSorting.Category) -> Void) {
self.init(passwordManagerCoordinator: PasswordManagerCoordinatingMock(),
onItemSelected: onItemSelected)
onItemSelected: onItemSelected,
onAddItemSelected: onAddItemSelected)
}

}

0 comments on commit e26ec38

Please sign in to comment.