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

Checklist: Simplify and de-duplicate #25924

Closed
wants to merge 13 commits into from
Closed

Checklist: Simplify and de-duplicate #25924

wants to merge 13 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jul 9, 2018

Rationale: We're going to want to insert 'pending' state for individual checklist items for https://github.com/Automattic/wp-calypso/projects/70, so it helps to be able to inject those at connect() level. This PR untangles things somewhat to prepare for that.

  • Checklist: Alphabetize ChecklistTask props
  • Checklist: Use times() instead of range().map()
  • Checklist: De-duplicate and simplify
    • Note that both onboardingChecklist.js and jetpack-checklist.js (yes, we should harmonize filenames, too) had a function duplicated (named onboardingTasks and jetpackTasks(), respectively). At closer look, that function was essentially merging two objects, and sorting them by key according to an array. I've reshuffled that functionality so that it makes more sense now IMO. Note this also means that we're using objects instead of arrays (the id fields becoming the keys) which I find lend themselves better to what we're trying to do here (e.g. _.merge() things).
  • Note that this PR also brings back the 'Backups and Scanning' step that was removed per Checklist (Jetpack): Remove 'Backups and Scanning' task #25872. The reason is that it is still sent by the REST API and would lead to a funky empty step in the UI with the new merge strategy. We are going to use this step to reflect VP installation status later.

Testing Instructions

(mostly copied from #25901)

Verify that the checklist still works, both for WP.com, and JP sites

  • Navigate to http://calypso.localhost:3000/checklist/:site
  • Verify that the loading state (pulsating placeholders) works fine: no errors, and it's only transient 🙂
  • Try completing a couple of steps. Verify that they are correctly marked as complete afterwards, and that their completions status persists across a reload.
  • For WP.com sites (!), verify that you can also tick off tasks even without actually completing them by clicking the circle on the left hand side (which will turn into a green checkmark). Verify that this also persists across a reload.
  • AFAICS, propType warnings aren't a regression but also present on master. Please verify for yourself 🙂

Verify that the devdocs example still works

  • http://calypso.localhost:3000/devdocs/blocks/checklist
  • Try completing a couple of steps. Verify that they are correctly marked as complete afterwards, and that their completions status persists across a reload.
  • Watch the console: no errors, just messages to inform you what step was clicked

Verify that the ChecklistBanner component (in the stats section) still works:

(WP.com only)

Apply the following patch below for easier testing

diff --git a/client/my-sites/stats/checklist-banner/index.jsx b/client/my-sites/stats/checklist-banner/index.jsx
index d405a13..0cfedd0 100644
--- a/client/my-sites/stats/checklist-banner/index.jsx
+++ b/client/my-sites/stats/checklist-banner/index.jsx
@@ -154,10 +154,6 @@ export class ChecklistBanner extends Component {
                const task = this.getNextTask();
                const percentage = Math.round( ( completed / total ) * 100 ) || 0;
 
-               if ( ! this.canShow() ) {
-                       return null;
-               }
-
                return (
                        <Card className="checklist-banner">
                                { siteId && <QuerySiteChecklist siteId={ siteId } /> }

Navigate to http://calypso.localhost:3000/stats/day/:site

image

Click the 'Do It' button, and verify that it takes you to the correct destination and starts a Guided Tour.
Click the 'View your checklist' link, and verify that it takes you to the checklist.

Verify that the Woo checklist still works:

  • Logged-in, navigate to http://calypso.localhost:3000/start/about?ref=calypso-selector
  • Tick the 'Sell products or collect payments' box and click continue.
  • Go thru the usual NUX flow (select a theme, a domain, and pay for your plan. You can skip adding G Suite).
  • When presented with the purple 'Thank You' page, click the 'Create your Store' button.
  • (The 'Building your Store' step can take a while.)
  • Click 'Set up my Store'. Another 'Building your Store' step follows.
  • When setting your address, it's important to enter a US or Canadian one.
  • You're now presented with the Store checklist! From now on, you can always return to this site by navigating to http://calypso.localhost:3000/store/<site>
  • Verify that the checklist works, i.e. clicking on individual steps will take you to the relevant UI.

image

@matticbot
Copy link
Contributor

Note that both `onboardingChecklist.js` and `jetpack-checklist.js` (yes, we should harmonize filenames, too) had a function duplicated (named `onboardingTasks` and `jetpackTasks()`, respectively). At closer look, that function was essentially merging two objects, and sorting them by key according to an array. I've reshuffled that functionality so that it makes more sense now IMO.
@ockham ockham force-pushed the cleanup/checklist branch from 6a99ce0 to 9213fc4 Compare July 9, 2018 12:16
@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jul 9, 2018
@ockham ockham requested a review from a team July 9, 2018 20:41
@ockham
Copy link
Contributor Author

ockham commented Jul 10, 2018

Closing in favor of #25953, #25955, and most relevantly, #25956.

@ockham ockham closed this Jul 10, 2018
@ockham ockham deleted the cleanup/checklist branch July 10, 2018 12:34
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 10, 2018
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.

2 participants