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: Untangle server and client list merging #25956

Merged
merged 9 commits into from
Jul 12, 2018

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jul 10, 2018

Simpler version of #25924 that retains using arrays for tasks (since @sirreal convinced me it's safer to use arrays for ordered lists 😉 ). This also has the benefit of being less invasive.

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.

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.

Testing Instructions

(Copied from #25924 -- relevant bits only)

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.

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.

@matticbot
Copy link
Contributor

@ockham ockham force-pushed the simplify/checklist-steps branch 2 times, most recently from d4e8819 to ec55ef6 Compare July 10, 2018 12:32
@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 10, 2018
@ockham ockham requested a review from a team July 10, 2018 12:33
@ockham ockham force-pushed the simplify/checklist-steps branch from 0ddc9cb to 7e2738c Compare July 10, 2018 15:15
@ockham
Copy link
Contributor Author

ockham commented Jul 11, 2018

Thanks for a21fbd4! I guess we should probably do the same with onboardingChecklist.js...

const JetpackChecklist = () => <Checklist />;

export default connect( ( state, { siteId } ) => ( {
plugins: getPluginsForSite( state, siteId, false ),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this prop connected? It's not clear to me how it's used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, this whole file shouldn't even be here!

@sirreal
Copy link
Member

sirreal commented Jul 11, 2018

Thanks for a21fbd4! I guess we should probably do the same with onboardingChecklist.js...

👍 Yep, I think so. Didn't have time and wanted to run it by you first.

@sirreal sirreal force-pushed the simplify/checklist-steps branch from a21fbd4 to 8b1f5b1 Compare July 12, 2018 09:43
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 12, 2018
@sirreal
Copy link
Member

sirreal commented Jul 12, 2018

I think this is good to go. I'm writing up the results of some related discussion as an issue and will share soon. Maybe we should get some testing help from other folks as I've gotten my hands dirty here.

@ockham ockham merged commit 0e615b5 into master Jul 12, 2018
@ockham ockham deleted the simplify/checklist-steps branch July 12, 2018 15:35
@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 12, 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.

3 participants