Hiding dashboard cards: add context menus #20384
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ class DashboardPostsListCardCell: UICollectionViewCell, Reusable { | |
|
|
||
| // MARK: Views | ||
|
|
||
| private let frameView = BlogDashboardCardFrameView() | ||
|
|
||
| lazy var tableView: UITableView = { | ||
| let tableView = PostCardTableView() | ||
|
|
@@ -68,14 +68,9 @@ class DashboardPostsListCardCell: UICollectionViewCell, Reusable { | |
| } | ||
|
|
||
| private func addSubviews() { | ||
| frameView.translatesAutoresizingMaskIntoConstraints = false | ||
| frameView.add(subview: tableView) | ||
|
|
||
| contentView.addSubview(frameView) | ||
| contentView.pinSubviewToAllEdges(frameView, priority: Constants.constraintPriority) | ||
| } | ||
|
|
@@ -104,23 +99,37 @@ extension DashboardPostsListCardCell { | |
| assertionFailure("Cell used with wrong card type") | ||
| return | ||
| } | ||
| addContextMenu(card: cardType, blog: blog) | ||
|
|
||
| viewModel = PostsCardViewModel(blog: blog, status: status, view: self) | ||
| viewModel?.viewDidLoad() | ||
| tableView.dataSource = viewModel?.diffableDataSource | ||
| viewModel?.refresh() | ||
| } | ||
|
|
||
| private func addContextMenu(card: DashboardCard, blog: Blog) { | ||
| guard FeatureFlag.personalizeHomeTab.enabled else { return } | ||
|
|
||
| frameView.onEllipsisButtonTap = { | ||
|
There was a problem hiding this comment. I'm curious about your thoughts.
I'm looking at each of the cells which repeat the same pattern of adding ellipsis button handling, hiding action, and tracking code. Although duplication is not necessarily evil, I'm wondering if in this situation it makes and is possible to have something reused. To describe the card as "personalizable" and then it will share common these functionality and layout traits. What do you think about that?
Is this a counter-argument to my question and it's difficult to reuse the same layout and functionality code between personalizable cards? There was a problem hiding this comment. Thanks for bringing this up. I did add some reuse but didn't want to go as far as refactoring the existing cards:
But there is clearly some duplication in how the context menus get added: 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)
])
}I could make it reusable fairly easily: extension DashboardStatsCardCell {
private func configureCard(for blog: Blog, in viewController: UIViewController) {
if FeatureFlag.personalizeHomeTab.enabled {
let menu = UIMenu(title: "", options: .displayInline, children: [
BlogDashboardHelpers.makeHideCardAction(for: .todaysStats, siteID: blog.dotComID?.intValue ?? 0)
])
frameView.setContextMenu(menu, card: .todaysStats)
}
// ...
}
}
extension BlogDashboardCardFrameView {
func setContextMenu(_ menu: UIMenu, card: DashboardCard) {
onEllipsisButtonTap = {
BlogDashboardAnalytics.trackContextualMenuAccessed(for: card)
}
ellipsisButton.showsMenuAsPrimaryAction = true
ellipsisButton.menu = menu
}
}This way, we'll remove the most obvious instances of duplication, and cards will retain complete control over how their menus are configured. Should I make this change in this PR? I have some other ideas for refactoring this and also Ultimately, I'll want this "Hide this" action to appear automatically based on which cards are personalizable, but that sounds a bit overcomplicated. I do want to look into it a bit more. There was a problem hiding this comment. I also already used the same approach for adding the ellipsis in #20393, which is consistent with other cards. I think it'll make sense to update them all at once to make it easy to review/test. There was a problem hiding this comment. Thanks for the insightful answer and your examples! Those
I think we could do that in other PRs as well. There are already a fair amount of changes, it will be easier to digest additional ones in a separate PR. |
||
| 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) | ||
| ]) | ||
| } | ||
|
|
||
| private func configureDraftsList(blog: Blog) { | ||
| frameView.title = Strings.draftsTitle | ||
| frameView.titleHint = Strings.draftsTitleHint | ||
| frameView.onHeaderTap = { [weak self] in | ||
| self?.presentPostList(with: .draft) | ||
| } | ||
| } | ||
|
|
||
| private func configureScheduledList(blog: Blog) { | ||
| frameView.title = Strings.scheduledTitle | ||
| frameView.onHeaderTap = { [weak self] in | ||
| self?.presentPostList(with: .scheduled) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import Foundation | ||
|
|
||
| struct BlogDashboardHelpers { | ||
| static func makeHideCardAction(for card: DashboardCard, siteID: Int) -> UIAction { | ||
| UIAction( | ||
| title: Strings.hideThis, | ||
| image: UIImage(systemName: "minus.circle"), | ||
| attributes: [.destructive], | ||
| handler: { _ in | ||
| BlogDashboardAnalytics.trackHideTapped(for: card) | ||
| BlogDashboardPersonalizationService(siteID: siteID) | ||
| .setEnabled(false, for: card) | ||
| }) | ||
| } | ||
|
|
||
| private enum Strings { | ||
| static let hideThis = NSLocalizedString("blogDashboard.contextMenu.hideThis", value: "Hide this", comment: "Title for the context menu action that hides the dashboard card.") | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These code removals from
BlogDashboardCardFrameVieware required for this change to work. Previously, setting both theonHeaderTapandonEllipsisButtonTapwas unsupported. It is now.