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: add empty state view #20385

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Hiding dashboard cards: add empty state view #20385

merged 1 commit into from
Mar 31, 2023

Conversation

kean
Copy link
Contributor

@kean kean commented Mar 22, 2023

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
  3. Hiding dashboard cards: add context menus #20384

There is only one commit in this PR: fb67fbd5.

Add an empty state view in case there are no cards to display on the Home tab.

To test:

  • Hide all cards from the dashboard
  • Verify that the new empty state view is displayed

Screenshot 2023-03-22 at 6 55 10 PM

Regression Notes

  1. Potential unintended areas of impact
    n/a (new feature, doesn't affect the existing cards)

  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)
    I covered the new logic for displaying the empty state view with 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.

@kean kean changed the title Feature/personalize home tab empty state Hiding dashboard cards: add empty state view Mar 22, 2023
case .personalize:
return AppConfiguration.isJetpack && FeatureFlag.personalizeHomeTab.enabled
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 removed AppConfiguration.isJetpack from here to make it easier to test using the WordPress test target. I'm planning to add AppConfiguration.isJetpack check to FeatureFlags instead one we are ready to enable it, the same way it's done for prompts, for example:

case .bloggingPrompts:
    return AppConfiguration.isJetpack

@kean kean marked this pull request as ready for review March 29, 2023 20:56
@@ -21,6 +21,8 @@ enum DashboardCard: String, CaseIterable {
/// Card placeholder for when loading data
case ghost
case failure
/// Empty state when no cards are present
case empty
Copy link
Contributor Author

@kean kean Mar 30, 2023

Choose a reason for hiding this comment

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

Other state views such as ghost and failure are also displayed using DashboardCard. I kept it consistent.

@osullivanchris
Copy link

How does the button look in dark mode? For the button background color I used 'fill color - tertiary' in the design. Which has a light mode and dark mode version. But realise I never specifically called that out.

@kean
Copy link
Contributor Author

kean commented Mar 30, 2023

You can find the dark mode screenshot in the previous PR #20369.

@osullivanchris
Copy link

@kean ah cool, looks good thanks!

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM 👍 Adapts well with dynamic type, dark/light mode, bigger screens, smaller screens, and different orientations.

I agree with using the cell to it consistent. Even though cards.isEmpty || cards.map(\.cardType) == [.personalize] such code is necessary, it looks like a special case to me compared to other cards.

@staskus
Copy link
Contributor

staskus commented Mar 31, 2023

@kean there're some conflicts. Let me know when you fix them, I will run CI and auto-merge this PR. Thanks!

@kean
Copy link
Contributor Author

kean commented Mar 31, 2023

@kean there're some conflicts. Let me know when you fix them, I will run CI and auto-merge this PR. Thanks!

Thanks for the review - I fixed the conflicts.

@staskus staskus enabled auto-merge (squash) March 31, 2023 14:14
@staskus staskus removed this from the 22.1 milestone Mar 31, 2023
@staskus staskus merged commit 10cc23e into wordpress-mobile:trunk Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants