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

Hiding dashboard cards: make quick start cards personalizable #20393

Merged
merged 3 commits into from
May 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ final class DashboardActivityLogCardCell: DashboardCollectionViewCell {
let activitySubmenu = UIMenu(title: String(), options: .displayInline, children: [activityAction])


let hideThisAction = BlogDashboardHelpers.makeHideCardAction(for: .activityLog,
siteID: blog.dotComID?.intValue ?? 0)
let hideThisAction = BlogDashboardHelpers.makeHideCardAction(for: .activityLog, blog: blog)

cardFrameView.ellipsisButton.menu = UIMenu(title: String(), options: .displayInline, children: [
activitySubmenu,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ final class BlogDashboardPersonalizeCardCell: DashboardCollectionViewCell {
}
WPAnalytics.track(.dashboardCardItemTapped, properties: ["type": DashboardCard.personalize.rawValue], blog: blog)
let viewController = UIHostingController(rootView: NavigationView {
BlogDashboardPersonalizationView(viewModel: .init(service: .init(siteID: siteID)))
BlogDashboardPersonalizationView(viewModel: .init(service: .init(siteID: siteID), quickStartType: blog.quickStartType))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered injecting the entire blog, but it would've made it harder to unit-test and create previews for this screen. I don't expect more parameters to be needed.

Copy link
Contributor

@staskus staskus Mar 31, 2023

Choose a reason for hiding this comment

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

Got it! Thanks for including the diff for an easier review.

The injection of quickStartType was the only question mark for me. However, although at first sight it did catch my attention, I think this approach is more correct even beyond the argument of making it easier to write unit-tests and create previews. We make BlogDashboardPersonalizationViewModel depend on only what it needs to depend on. Due to business requirements, a quick start does require special handling.

I don't expect more parameters to be needed.

Yes, it doesn't look to be needed for the existing cards. In the future, who knows. If more and more custom behaviour would be needed, we could consider some different approach which wouldn't require injecting all the different dependencies into personalization VM and maybe would be handled by some configuration of each card.

}.navigationViewStyle(.stack)) // .stack is required for iPad
if UIDevice.isPad() {
viewController.modalPresentationStyle = .formSheet
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,10 @@ extension DashboardPagesListCardCell {
}
cardFrameView.ellipsisButton.showsMenuAsPrimaryAction = true

let children = [makeAllPagesAction(), makeHideCardAction(blog: blog)].compactMap { $0 }
let children = [
makeAllPagesAction(),
BlogDashboardHelpers.makeHideCardAction(for: .pages, blog: blog)
].compactMap { $0 }

cardFrameView.ellipsisButton.menu = UIMenu(title: String(), options: .displayInline, children: children)
}
Expand All @@ -119,13 +122,6 @@ extension DashboardPagesListCardCell {
return allPagesSubmenu
}

private func makeHideCardAction(blog: Blog) -> UIMenuElement? {
guard let siteID = blog.dotComID?.intValue else {
return nil
}
return BlogDashboardHelpers.makeHideCardAction(for: .pages, siteID: siteID)
}

// MARK: Actions

private func showPagesList(source: PagesListSource) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,15 @@ class BlogDashboardCardFrameView: UIView {
buttonContainerStackView.removeFromSuperview()
}

/// Adds the "more" button with the given actions to the corner of the cell.
func addMoreMenu(items: [UIMenuElement], card: DashboardCard) {
onEllipsisButtonTap = {
BlogDashboardAnalytics.trackContextualMenuAccessed(for: card)
}
ellipsisButton.showsMenuAsPrimaryAction = true
ellipsisButton.menu = UIMenu(title: "", options: .displayInline, children: items)
}

private func updateEllipsisButtonState() {
ellipsisButton.isHidden = onEllipsisButtonTap == nil
let headerPadding = ellipsisButton.isHidden ?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,9 @@ extension DashboardPostsListCardCell {
private func addContextMenu(card: DashboardCard, blog: Blog) {
guard FeatureFlag.personalizeHomeTab.enabled else { return }

frameView.onEllipsisButtonTap = {
BlogDashboardAnalytics.trackContextualMenuAccessed(for: card)
}
frameView.ellipsisButton.showsMenuAsPrimaryAction = true
frameView.ellipsisButton.menu = UIMenu(title: "", options: .displayInline, children: [
BlogDashboardHelpers.makeHideCardAction(for: card, siteID: blog.dotComID?.intValue ?? 0)
])
frameView.addMoreMenu(items: [
BlogDashboardHelpers.makeHideCardAction(for: card, blog: blog)
], card: card)
}

private func configureDraftsList(blog: Blog) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,27 +51,33 @@ final class DashboardQuickStartCardCell: UICollectionViewCell, Reusable, BlogDas
fallthrough

case .newSite:
configureOnEllipsisButtonTap(sourceRect: cardFrameView.ellipsisButton.frame)
configureOnEllipsisButtonTap(sourceRect: cardFrameView.ellipsisButton.frame, blog: blog)
cardFrameView.showHeader()

case .existingSite:
cardFrameView.configureButtonContainerStackView()
configureOnEllipsisButtonTap(sourceRect: cardFrameView.buttonContainerStackView.frame)
configureOnEllipsisButtonTap(sourceRect: cardFrameView.buttonContainerStackView.frame, blog: blog)
cardFrameView.hideHeader()

}

cardFrameView.setTitle(Strings.title(for: blog.quickStartType))
}

private func configureOnEllipsisButtonTap(sourceRect: CGRect) {
cardFrameView.onEllipsisButtonTap = { [weak self] in
guard let self = self,
let viewController = self.viewController,
let blog = self.blog else {
return
private func configureOnEllipsisButtonTap(sourceRect: CGRect, blog: Blog) {
if FeatureFlag.personalizeHomeTab.enabled {
cardFrameView.addMoreMenu(items: [
BlogDashboardHelpers.makeHideCardAction(for: .quickStart, blog: blog)
], card: .quickStart)
} else {
cardFrameView.onEllipsisButtonTap = { [weak self] in
guard let self = self,
let viewController = self.viewController,
let blog = self.blog else {
return
}
viewController.removeQuickStart(from: blog, sourceView: self.cardFrameView, sourceRect: sourceRect)
}
viewController.removeQuickStart(from: blog, sourceView: self.cardFrameView, sourceRect: sourceRect)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,9 @@ extension DashboardStatsCardCell: BlogDashboardCardConfigurable {
}

if FeatureFlag.personalizeHomeTab.enabled {
frameView.onEllipsisButtonTap = {
BlogDashboardAnalytics.trackContextualMenuAccessed(for: .todaysStats)
}
frameView.ellipsisButton.showsMenuAsPrimaryAction = true
frameView.ellipsisButton.menu = UIMenu(title: "", options: .displayInline, children: [
BlogDashboardHelpers.makeHideCardAction(for: .todaysStats, siteID: blog.dotComID?.intValue ?? 0)
])
frameView.addMoreMenu(items: [
BlogDashboardHelpers.makeHideCardAction(for: .todaysStats, blog: blog)
], card: .todaysStats)
}

statsStackView?.views = viewModel?.todaysViews
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ enum DashboardCard: String, CaseIterable {
.scheduledPosts,
.blaze,
.prompts,
.quickStart,
.pages,
.activityLog
]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import Foundation

struct BlogDashboardHelpers {
static func makeHideCardAction(for card: DashboardCard, siteID: Int) -> UIAction {
static func makeHideCardAction(for card: DashboardCard, blog: Blog) -> UIAction {
UIAction(
title: Strings.hideThis,
image: UIImage(systemName: "minus.circle"),
attributes: [.destructive],
handler: { _ in
BlogDashboardAnalytics.trackHideTapped(for: card)
BlogDashboardPersonalizationService(siteID: siteID)
BlogDashboardPersonalizationService(siteID: blog.dotComID?.intValue ?? 0)
.setEnabled(false, for: card)
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ private func makeKey(for card: DashboardCard) -> String? {
return "activity-log-card-enabled-site-settings"
case .pages:
return "pages-card-enabled-site-settings"
case .quickStart, .jetpackBadge, .jetpackInstall, .nextPost, .createPost, .failure, .ghost, .personalize, .empty:
case .quickStart:
return "quick-start-card-enabled-site-settings"
case .jetpackBadge, .jetpackInstall, .nextPost, .createPost, .failure, .ghost, .personalize, .empty:
return nil
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ private extension BlogDashboardPersonalizationView {
struct BlogDashboardPersonalizationView_Previews: PreviewProvider {
static var previews: some View {
NavigationView {
BlogDashboardPersonalizationView(viewModel: .init(service: .init(repository: UserDefaults.standard, siteID: 1)))
BlogDashboardPersonalizationView(viewModel: .init(service: .init(repository: UserDefaults.standard, siteID: 1), quickStartType: .newSite))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@ import SwiftUI
final class BlogDashboardPersonalizationViewModel: ObservableObject {
let cards: [BlogDashboardPersonalizationCardCellViewModel]

init(service: BlogDashboardPersonalizationService) {
self.cards = DashboardCard.personalizableCards.map {
BlogDashboardPersonalizationCardCellViewModel(card: $0, service: service)
init(service: BlogDashboardPersonalizationService, quickStartType: QuickStartType) {
self.cards = DashboardCard.personalizableCards.compactMap {
if $0 == .quickStart && quickStartType == .undefined {
return nil
}
let title = $0.getLocalizedTitle(quickStartType: quickStartType)
return BlogDashboardPersonalizationCardCellViewModel(card: $0, title: title, service: service)
}
}
}
Expand All @@ -15,7 +19,7 @@ final class BlogDashboardPersonalizationCardCellViewModel: ObservableObject, Ide
private let service: BlogDashboardPersonalizationService

var id: DashboardCard { card }
var title: String { card.localizedTitle }
let title: String

var isOn: Bool {
get { service.isEnabled(card) }
Expand All @@ -25,14 +29,15 @@ final class BlogDashboardPersonalizationCardCellViewModel: ObservableObject, Ide
}
}

init(card: DashboardCard, service: BlogDashboardPersonalizationService) {
init(card: DashboardCard, title: String, service: BlogDashboardPersonalizationService) {
self.card = card
self.title = title
self.service = service
}
}

private extension DashboardCard {
var localizedTitle: String {
func getLocalizedTitle(quickStartType: QuickStartType) -> String {
switch self {
case .prompts:
return NSLocalizedString("personalizeHome.dashboardCard.prompts", value: "Blogging prompts", comment: "Card title for the pesonalization menu")
Expand All @@ -48,7 +53,17 @@ private extension DashboardCard {
return NSLocalizedString("personalizeHome.dashboardCard.activityLog", value: "Recent activity", comment: "Card title for the pesonalization menu")
case .pages:
return NSLocalizedString("personalizeHome.dashboardCard.pages", value: "Pages", comment: "Card title for the pesonalization menu")
case .quickStart, .nextPost, .createPost, .ghost, .failure, .personalize, .jetpackBadge, .jetpackInstall, .domainsDashboardCard, .freeToPaidPlansDashboardCard, .domainRegistration, .empty:
case .quickStart:
switch quickStartType {
case .undefined:
assertionFailure(".quickStart card should only appear in the personalization menu if there are remaining steps")
return ""
case .existingSite:
return NSLocalizedString("personalizeHome.dashboardCard.getToKnowTheApp", value: "Get to know the app", comment: "Card title for the pesonalization menu")
case .newSite:
return NSLocalizedString("personalizeHome.dashboardCard.nextSteps", value: "Next steps", comment: "Card title for the pesonalization menu")
}
case .nextPost, .createPost, .ghost, .failure, .personalize, .jetpackBadge, .jetpackInstall, .empty, .domainsDashboardCard, .freeToPaidPlansDashboardCard, .domainRegistration:
assertionFailure("\(self) card should not appear in the personalization menus")
return "" // These cards don't appear in the personalization menus
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,12 @@ final class BlogDashboardPersonalizationServiceTests: XCTestCase {
// Then settings for site 1 are ignored
XCTAssertTrue(service.isEnabled(.quickStart))
}

func testThatUserDefaultsKeysAreSpecifiedForAllPersonalizableCards() {
let service = BlogDashboardPersonalizationService(repository: repository, siteID: 1)
for card in DashboardCard.personalizableCards {
service.setEnabled(false, for: card)
XCTAssertFalse(service.isEnabled(card))
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ final class BlogDashboardPersonalizationViewModelTests: XCTestCase {
override func setUp() {
super.setUp()

viewModel = BlogDashboardPersonalizationViewModel(service: service)
viewModel = BlogDashboardPersonalizationViewModel(service: service, quickStartType: .undefined)
}

func testThatCardStateIsToggled() throws {
Expand All @@ -30,4 +30,22 @@ final class BlogDashboardPersonalizationViewModelTests: XCTestCase {
XCTAssertFalse(cardViewModel.isOn)
XCTAssertFalse(service.isEnabled(card), "Service wasn't updated")
}

func testThatAllCardsHaveTitles() {
for card in viewModel.cards {
XCTAssertTrue(!card.title.isEmpty)
}
}

func testThatQuickStartCardsIsNotDisplayedWhenTourIsActive() {
// Given
viewModel = BlogDashboardPersonalizationViewModel(service: service, quickStartType: .newSite)

// Then
XCTAssertTrue(viewModel.cards.contains(where: { $0.id == .quickStart }))
}

func testThatQuickStartCardsIsNotDisplayedWhenNoTourIsActive() {
XCTAssertFalse(viewModel.cards.contains(where: { $0.id == .quickStart }))
}
}