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

My Site Dashboard - Phase 2 - Error Card Retry #15763

Merged

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Dec 23, 2021

Parent #15625

This PR adds retry functionality to the dashboard error card and fine-tunes the error card UI.

UI

Light Mode Dark Mode
error_card_light error_card_dark

To test:

Prerequisites

  • Go to My Site -> Me -> App Settings -> Test Feature Configuration.
  • Turn the MySiteDashboardPhase2FeatureConfig feature flag ON.
  • Relaunch the app.
  1. Update the CardsSource.getData(...) and CardsSource.fetchCardsAndPostErrorIfAvailable(...) logic to get the showErrorCard to true.
  2. Re-install the app.
  3. Notice that the error card is displayed with a retry button.
  4. Click the retry button.
  5. Notice that the My Site tab is refreshed.

Merge Instructions

  • Make sure that the PR is design-reviewed
  • Remove Needs Design Review and Not Ready for Merge labels
  • Merge the PR

Regression Notes

  1. Potential unintended areas of impact
  • Changes have a limited scope, only adds a retry button functionality and updates the styles.
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • See above.
  1. What automated tests I added (or what prevented me from doing so)
  • Added unit test to verify refresh action.

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

@ashiagr ashiagr added [Status] Needs Design Review A designer needs to sign off on the implemented design. [Status] Not Ready for Merge My Site Dashboard labels Dec 23, 2021
@ashiagr ashiagr added this to the 19.0 milestone Dec 23, 2021
@ashiagr ashiagr self-assigned this Dec 23, 2021
@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@ashiagr ashiagr mentioned this pull request Dec 23, 2021
13 tasks
@ParaskP7 ParaskP7 self-assigned this Dec 23, 2021
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @ashiagr !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟

PS.1: Feel free to merge this PR when CI completes and Chris gives you the 🟢 light as well.
PS.2: Pair programming rocks! 🕺 💃

@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APKs:

Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

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

@ashiagr Hey it looks spot on, nice job.

I wrote the copy myself and its not great. So I've added the needs copy review label. We can go ahead and merge and tweak the copy later if we get feedback though.

FAO editorial:

  • we're making a new dynamic dashboard in the My Site tab of the WordPress apps
  • we show information about users posts, stats, other things
  • this is an error state if we fail to load the dynamic content. Noting the content/dashboard doesn't have a name. So we shouldn't refer to the dashboard by name exactly I don't think, just note that something didn't load.
  • Some other content is still on the page which didn't need to be fetched from the same place.

@ashiagr
Copy link
Contributor Author

ashiagr commented Dec 23, 2021

Thank you, @ParaskP7, for sharing all those helpful tips in our pair programming!
@osullivanchris, thanks to you too for taking a look at short notice!

I'll merge this PR and once we hear from the editorial, will open another one for any copy changes.

@ashiagr ashiagr removed [Status] Needs Design Review A designer needs to sign off on the implemented design. [Status] Not Ready for Merge labels Dec 23, 2021
@ashiagr ashiagr merged commit 01aa93a into develop Dec 23, 2021
@ashiagr ashiagr deleted the issue/issue/15625-page-behaviours-error-state-part-iii branch December 23, 2021 14:09
@kristastevens
Copy link

Hi @osullivanchris, thanks for the ping!

I have a suggestion for that middle line:

Some data hasn't loaded
We're having trouble loading your site's data at the moment.
Button: Retry

@osullivanchris
Copy link

@kristastevens thanks for taking a look. That new copy sounds good to me!

@ashiagr is it ok to do that copy tweak in another PR?

@ashiagr
Copy link
Contributor Author

ashiagr commented Dec 27, 2021

@ashiagr is it ok to do that copy tweak in another PR?

Sounds good to me @osullivanchris! 🙌 I'll create another PR for the copy change.

@ParaskP7
Copy link
Contributor

ParaskP7 commented Jan 3, 2022

👋 @ashiagr @osullivanchris !

FYI: I have created this #15785 issue to deal with updating the copy.

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.

4 participants