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

ChecklistShow: Decouple tasks prop #25958

Closed
wants to merge 6 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jul 10, 2018

Move the responsibility to provide a tasks array prop to the Checklist prop from the ChecklistShow wrapper to its consumer. The rationale is that for https://github.com/Automattic/wp-calypso/projects/70, the logic providing that prop will diverge for JP sites from how we're doing it for WP.com ones, since in the Jetpack case, we want to display 'pending' state for some steps (spinners), which we'll infer from selectors (essentially plugin installation state, see #25935). To that end, we want to be able to transparently extend the tasks array in the connect() that is relevant for the JP case.

Note that this PR changes behavior: the checklist for JP sites is now only available at /plans/my-plan/:site, whereas /checklist/:site will no longer show a checklist for JP sites (only for WP.com).

Testing Instructions

Verify that the checklist still works for WP.com sites

  • Navigate to http://calypso.localhost:3000/checklist/<wpcomSite>
  • 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.
  • 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.
  • Try switching to a JP site -- you'll see a message "Checklist not available for this site" (We will add a redirect to /plans/my-plan/<jpSite> going forward.)

Verify that the checklist still works for Jetpack sites

  • Navigate to http://calypso.localhost:3000/checklist/<jpSite>
  • 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.
  • Verify that propType errors in the console are pre-existing on master.
  • Try switching to a WP.com site, and verify that no checklist is being displayed.

@matticbot
Copy link
Contributor

@ockham ockham force-pushed the update/checklist-show-decouple-tasks branch from 8dc414b to aa89a49 Compare July 10, 2018 14:52
@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 15:13
@sirreal sirreal force-pushed the update/checklist-show-decouple-tasks branch from aa89a49 to 35fa110 Compare July 12, 2018 12:43
@sirreal
Copy link
Member

sirreal commented Jul 12, 2018

Rebased (new node version)

@ockham ockham force-pushed the update/checklist-show-decouple-tasks branch from 35fa110 to 103fa35 Compare July 12, 2018 15:43
@ockham
Copy link
Contributor Author

ockham commented Jul 12, 2018

Rebased

@sirreal
Copy link
Member

sirreal commented Jul 12, 2018

I'm not sure this aligns with the direction outlined in #26015

Before reviewing this fully, I'm going to do some exploration to see what implementing those changes might look like.

@sirreal
Copy link
Member

sirreal commented Jul 20, 2018

I'd like to go with #26216 over this, it's about ready 😁

@sirreal sirreal added [Status] Blocked / Hold and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 23, 2018
@sirreal sirreal deleted the update/checklist-show-decouple-tasks branch August 16, 2018 17:30
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