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: register cards automatically #20466

Closed
wants to merge 1 commit into from
Closed

Hiding dashboard cards: register cards automatically #20466

wants to merge 1 commit into from

Conversation

kean
Copy link
Contributor

@kean kean commented Apr 3, 2023

Related issue: #20296

Addresses the feedback by @staskus and @hassaanelgarem about simplifying how the cards are added to the Personalize Home Tab screen.

Please note that it also changes the order of cards as they appear in the Personalize Home Tab. It was previously based on Figma and is now based on how they appear on Dashboard. I confirmed this change here.

To test:

Follow the test instructions from #20369 (regression testing needed).

Regression Notes

  1. Potential unintended areas of impact
    The new Personalize Home Tab screen

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

  3. What automated tests I added (or what prevented me from doing so)
    It has existing unit tests

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.

@@ -4,7 +4,9 @@ final class BlogDashboardPersonalizationViewModel: ObservableObject {
let cards: [BlogDashboardPersonalizationCardCellViewModel]

init(service: BlogDashboardPersonalizationService) {
self.cards = DashboardCard.personalizableCards.map {
self.cards = BlogDashboardPersonalizationService.personalizableCards.filter {
$0 != .domainsDashboardCard
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staskus I temporarily hid the new card from the menu. Unlike other cards in the Personalize screen, it's shown only in a limited set of scenarios (has to be admin, etc). I'll defer to you to add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the cards only appear in special scenarios.

For example:

  1. Jetpack install when the Jetpack is connected without full plugin
  2. Quick-start when users choose to do it
  3. Blaze, when the blog supports it

How does Domains card differ from other scenarios?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pinging me, by the way!

Copy link
Contributor Author

@kean kean Apr 3, 2023

Choose a reason for hiding this comment

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

Good point.

  • Jetpack Install has a "Hide" action, but we decided not to add it to the Personalization screen because it's a one-time card. There is no value in unhiding it.
  • QuickStart is currently displayed only when there is an active tour, even if it's completed so that you could revisit it.
  • Blaze is currently always present in the Personalization menu. I wonder if we should change it, @osullivanchris?

In general, we should probably reconsider the logic behind showing cards in the Personalize menu. It's more nuanced than the Stats personalization screen.

Paul also raised a related concern last week:

Trying this out, I can see how the personalization options for the "draft post" card and "scheduled post" dashboard cards can be unintuitive when there is no draft or scheduled post. Perhaps this needs to be made more intuitive before we release this. I don't think this is a blocker to this PR, but worth addressing before we ship this to users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the answer.

One of the ideas to explore would be only showing the card in the personalization view if it belongs to personalizableCards and if DashboardCard.shouldShow(..) is true. Therefore it could be only personalized if:

  1. Card is eligible to be shown
  2. Card can be hidden

Copy link
Contributor

Choose a reason for hiding this comment

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

only showing the card in the personalization view if it belongs to personalizableCards and if DashboardCard.shouldShow(..) is true

This will introduce a problem for dynamically shown cards, like drafts, scheduled, createPost and newPost. Showing these cards depend on the user's posts. I think that inconsistently showing these cards in the personalize menu can confuse the user.

I think the logic for hiding specific cards should not be here, as this won't scale nicely. The logic to show/hide a card in the personalize menu should be encapsulated in personalizableCards, and the logic should be defined by the developer for each card. Either always show it, always hide it, or show it when applicable.

If we go that route, though, I think it would be simpler to have an isPersonalizable variable and automatically generate the key (With special handling for the Domains card for backward compatibility). What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that inconsistently showing these cards in the personalized menu can confuse the user.

I thought about it as well. We either choose to allow personalizing all the cards, with the drawback of not always showing the same cards in the personalization menu, OR we only allow personalizing some of the cards.

I think the logic for hiding specific cards should not be here, as this won't scale nicely.

I agree. If each card has its own personalization logic, it should be close to the card. 👍

Choose a reason for hiding this comment

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

Very interesting conversation. Thanks for shedding light and stress testing this.

I think there are a handful of categories for availability of cards.

  1. Always: Stats card is always available.
  2. Depending on Content: Draft posts depends on whether content is available
  3. User decision: Onboarding card depends on user accepting it initially.
  4. SiteType/User Role/Plan: Blaze card depends no eligibility of the blog. Activity Log depends on the user credentials. Some other cards in future can depend on whether the user is paying for a feature.

I don't think there is unlimited variations. I think we will exhaust the possible categories soon, or loosely be able to group into these categories.

As we go through the categories there, personalisation makes less sense going down:

  1. Definitely yes
  2. Probably yes
  3. Maybe
  4. No

I think the user should be able to turn on/off cards which are available to them. Which covers categories 1-3. I'd be open to debate on (3), but I've erred on the side that gives the user more choice and ability to undo a hide action.

For (4), this card is not even available to them to begin with. This is like a gate before what we're doing. If they don't pass the first gate, don't give them control at all. There is edge cases where a card becomes unavailable and then available again (e.g. we have a card depending on a paid plan. The user upgrades then downgrades). We could argue to keep the previous status in this case. But I think its overkill. If the user doesn't have access to Blaze, there's no personalisation option. If a new card becomes available, it defaults to 'on', and its now in the personalisation options.

For category 4 I did have this mockup where we could indicate cards that are unavailable, perhaps even consider upselling there. But I thought it was simpler to leave out for v1. I'm now realising that even if we leave it out of the UI, we still need to deal with it in the back end logic.

Screenshot 2023-04-04 at 09 51 46

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 think the logic for hiding specific cards should not be here, as this won't scale nicely. The logic to show/hide a card in the personalize menu should be encapsulated in personalizableCards, and the logic should be defined by the developer for each card.

I agree. Currently, the Personalization screen's ViewModel determines which cards to show, which I think is OK. It could be generalized, but I decided not to tackle it yet – not until the display logic is clear.

The code that determines whether the card is eligible for the current blog&user could be extracted from DashboardCard.shouldShow to a separate method (DashboardCard.isEligible(blog:user:)). Then the personalization screen could contain cards that are personalizable & eligible. But then there are also cards like Quick Tours. Every blog is eligible to show these, but, as Chris pointed out, it's based on the user's decision, so it gets more complicated.

I think it would be simpler to have an isPersonalizable variable and automatically generate the key

That's what I started with, but it was changed to static keys based on the feedback. Rationale in #20365.

still need to deal with it in the back end logic.

It's covered for all cards except for Blaze – this one needs to be shown based on BlazeHelper.shouldShowCard(for: blog). I put it on the list of things to address.

Copy link
Contributor

Choose a reason for hiding this comment

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

the Personalization screen's ViewModel determines which cards to show

Not really. The way I understand it, this logic is split between personalizableCards and the filter in BlogDashboardPersonalizationViewModel's init.

Then the personalization screen could contain cards that are personalizable & eligible

As Chris pointed out, there are multiple categories to consider, and there may be more in the future. For that reason, I wouldn't advocate for a general solution for all cards. I think whether the card is personalizable should be clearly defined for each card. We can still create helpers like isEligible(), for example, and use them to define if the card is personalizable.

That's what I started with, but it was changed to static keys based on the feedback. Rationale in #20365.

Thanks for sharing the context! The feedback over there makes sense. But given that we're now depending on makeKey to check if the card is personalizable, and we're discussing adding more logic to what it means to be personalizable, then maybe we should reconsider this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants