Hiding dashboard cards: add context menus

[kean]

Related issue: #20296

Previous PRs (dependencies):

1. Hiding dashboard cards: generalize logic for storing preferences #20365
2. Hiding dashboard cards: add Personalize Home Tab screen #20369

Changes

• Add context menus with the "Hide this" button to "Drafts", "Scheduled Posts", and "Today's Stats" cards. Now every card from the new "Personalize Home Tab" screen has a "Hide this" button in the context menu. A couple more cards require special attention and will be made "personalizable" later.
• Remove unused code left after the recent dashboard cards redesign (already confirmed that these can be removed)
• Add generalized tracking for context menus and "Hide this" actions for cards (see BlogDashboardAnalytics)

Screenshots

Testing

Prerequisites

• "Personalize Home Tab" feature flag is enabled

Test Case 1 (Draft Post)

• Create a draft post and open dashboard
• Verify that the "Drafts" card now has a context menu
• Tap the context menu and select "Hide this"
• Verify that the card is hidden

Test Case 2 (Schedule Post)

• Create a scheduled post and open dashboard
• Verify that the "Schedules Posts" card now has a context menu
• Tap the context menu and select "Hide this"
• Verify that the card is hidden

Test Case 3 (Both)

• Create both a draft and a scheduled post
• Hide drafts
• Verify that schedules posts are still visible

Test Case 4 (Stats)

• Verify that the "Today's Stats" card has a context menu with the "Hide This" button
• Tap the button and verify that the card got hidden

Test Case 5 (Stored Per-Site)

• Create a draft for site A and hide the "Drafts" card
• Switch to site B and create a draft
• Verify that the "Drafts" cards is visible on Dashboard

Test Case 6 (Regression)

• Create a draft post
• Verify that the header and the individual posts are still tappable separately
• Verify that the context menu button is easy to tap with a finder (should preferably be testesd on the physical device)

Other Testing Notes:

• Test on iOS 14 to make sure context menus are displayed correctly
• Test with large dynamic type to make sure card titles don't overlap with the context menus
• An easy way to test all cards layout is by removing filter in BlogDashboardService.parse

Regression Notes

1. Potential unintended areas of impact
   This change affects primarily "Drafts," "Schedules Posts", and "Today's Stats" cards, but other dashboard cards were also affected. It must be verified that the context menu layout wasn't broken for other cards, such as Blaze.

2. What I did to test those areas of impact (or what existing automated tests I relied on)
   Manual testing

3. What automated tests I added (or what prevented me from doing so)
   n/a

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[kean]

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

[osullivanchris]

@kean Hey there. Just a nice to have idea here. Could we change the ellipsis icon to one from our new icon set? I've been holding off on asking, as we actually need to do this globally. But it will look so much better in this instance, and its a new introduction, that it would be nice to use the new ellipsis.

It can be found here under more-horizontal-mobile https://www.npmjs.com/package/@wordpress/icons

Or I'll attach an SVG below

I've been using it with secondary label color like the below mock.

If this is more than like 30-60mins work, then don't add it to the scope, I can get it done as part of another upcoming project

[kean]

Sure, it's a small change. I have a list of minor tweaks I need to make. I'll create a separate PR with these changes.

[staskus]

Thanks for the work, @kean! I tested it, and the functionality works as described 👍

My biggest question is about the whole approach. Whether we take the same steps for every personalizable card or is it worth describing them once at a deeper level? Interested in your thoughts.

[staskus]

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?

[kean]

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.

[kean]

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.

[staskus]

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.

[staskus]

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.

[kean]

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.

[staskus]

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. 👍

[staskus]

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

[staskus]

✅ Looks good now!

As we discussed, let's leave potential refactoring and improved reusability for another PR so we could discuss it separately.

[kean]

@staskus. There was another merge conflict. I fixed it, and it canceled the auto-merge. I'd appreciate it if you could re-run it.