These code removals from BlogDashboardCardFrameView are required for this change to work. Previously, setting both the onHeaderTap and onEllipsisButtonTap was unsupported. It is now.

I'm curious about your thoughts.

Now every card from the new "Personalize Home Tab" screen has a "Hide this" button in the context menu.

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?

A couple more cards require special attention and will be made "personalizable" later.

Is this a counter-argument to my question and it's difficult to reuse the same layout and functionality code between personalizable cards?

Thanks for bringing this up. I did add some reuse but didn't want to go as far as refactoring the existing cards:

• Creating a UIAction to hide a card for new cards is fully reused
• Tracking the hide action is also reused

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 Dashboard.personalizableCards. I was going to open a new PR after getting the features merged first.

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.

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.

Thanks for the insightful answer and your examples! Those extensions look like good wins to take.

Should I make this change in this PR? I have some other ideas for refactoring this and also Dashboard.personalizableCards. I was going to open a new PR after getting the features merged first.

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.

I tested the changes on the latest trunk as I was interested in how these changes affect the latest development.

The top of 4 for a multiline label looks visually too close. It looks like a title label centers vertically together with the button. When we have a single-line label, the top of 4 looks fine, and 8 is too far.

Thanks, good catch. I tested only with the existing cards, which are single-line. titleLabel.numberOfLines = 0 was just added to trunk this week.

I reverted this change for now. It's a production behavior, and I don't want this change to hold the main PRs. I'll think of a different way to implement it. I could do it using hitTest, then there won't be any layout changes.

Thanks, retested it, looks okay now, although yes, now the button appears a bit too low, so this part as you mentioned could be improved. 👍

The cut-off for 22.1 is on Monday and the feature is still under the feature flag and development.

I think it's safer to move these notes to 22.2 and aim to enable those changes then.

Does that sound good?

CC: @guarani