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

Jetpack Checklist: Make task list dynamic #31547

Merged
merged 6 commits into from
Apr 5, 2019

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Mar 19, 2019

Currently, the Jetpack checklist has static tasks. While we'll still have static copy and actions for each task, this PR updates the task list to use the tasks as fetched from the server.

Changes proposed in this Pull Request

  • Make the Jetpack checklist dynamic.

Preview

Some tasks finished:

Akismet setting up:

Akismet finished:

Akismet task not available for free plan:

Testing instructions

  • Checkout this branch.
  • Create a JN site with the latest bleeding edge.
  • Connect a Jetpack site (add &calypso_env=development to the connection URL to point it to Calypso).
  • Buy a plan.
  • You'll end up in the My plan page with the checklist and the plan setup screen.
  • Verify the backups, monitor, plugin updates and WP.com sign in tasks work well and reflect the current state of the task.
  • Verify Akismet task is in progress while Akismet is being setup, and it gets to a finished state when it finishes installing.
  • Test with a site with a free plan and verify Akismet task isn't shown.

Next steps

There are still some unfinished things, bugs and edge cases to iron out, for example:

  • The VP task doesn't work well yet and needs to be implemented differently for non-Rewind sites.
  • Task state gets cached and doesn't get reflected upon uncompletion.

Those will be fixed subsequently in further PRs and diffs.

@tyxla tyxla added Jetpack [Pri] Normal Schedule for the next available opportuinity. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. NUX Checklist [Type] Feature progress labels Mar 19, 2019
@tyxla tyxla self-assigned this Mar 19, 2019
@matticbot
Copy link
Contributor

@tyxla tyxla requested a review from a team March 19, 2019 10:37
Copy link
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

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

This worked great in my testing! The tasklist was automatically updated after completing the tasks on it.

tasklist-01

The only I noticed is that the progress bar doesn't auto update when turning off features, though the number does:

tasklist-02

@tyxla
Copy link
Member Author

tyxla commented Mar 25, 2019

Thanks for testing @keoshi!

The only I noticed is that the progress bar doesn't auto update when turning off features, though the number does:

I've tried this, and while the number took a little more to update, it eventually did update a couple of seconds after coming back to the checklist page. Any chance you just didn't wait enough for it to update?

@keoshi
Copy link
Contributor

keoshi commented Mar 25, 2019

@tyxla Maybe I wasn't clear enough, sorry about that. What I mean is that the complete portion on the progress bar (green part) did not update after turning off features. The number (3/6) was updated correctly once I opened the task list, but the progress bar was updated only after a page refresh. The second GIF captures this scenario.

@tyxla
Copy link
Member Author

tyxla commented Mar 26, 2019

@keoshi nice catch! This is caused by the 2 tasks that remain static. I'd very much like to make them dynamic, but that can happen after we land #31684 and have accurate information about VP/AK the setup status and progress. So that'll be addressed in a follow up PR.

@keoshi
Copy link
Contributor

keoshi commented Mar 26, 2019

Makes total sense, Marin! 👍

@tyxla tyxla force-pushed the update/jetpack-checklist-dynamic-tasks branch from f89f5e8 to 6b867c6 Compare April 4, 2019 09:48
@tyxla
Copy link
Member Author

tyxla commented Apr 4, 2019

Now that we have information for the progress of the AK/VP tasks, I'll work on making those dynamic as well.

@tyxla tyxla added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 4, 2019
@tyxla tyxla force-pushed the update/jetpack-checklist-dynamic-tasks branch from 6b867c6 to 7e13f66 Compare April 4, 2019 12:56
@tyxla
Copy link
Member Author

tyxla commented Apr 4, 2019

The VP task is still under some discussion, but I've just made the Akismet one dynamic by:

  • Adding a QueryJetpackProductInstallStatus query component.
  • Making the Akismet task in progress or completed, based on the installation status:

Akismet setting up:

Akismet Finished:

  • Making the Akismet task available only for paid plans:

@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Apr 4, 2019
@tyxla tyxla requested a review from a team April 4, 2019 13:48
@tyxla
Copy link
Member Author

tyxla commented Apr 4, 2019

This is ready for code and design review.

I'm satisfied with this PR - while there are things to fix and iterate on, they'll be addressed in future PRs (this is under a feature flag).

@jeffgolenski
Copy link
Member

This is looking great, @tyxla. As promised, here's a quick video walkthrough. the functionality is great, I just make note of some visual changes I think we can make to improve the clarity of the checklist.

https://cloudup.com/cYvYFPwMpWH

I'll create issues with some designs for that stuff later.

@tyxla tyxla removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Apr 4, 2019
@enejb
Copy link
Member

enejb commented Apr 5, 2019

@jeffgolenski Just watched the walk though. Some great work there @tyxla and @jeffgolenski!

One thing that stood out to me is that there was an activity in the activity log that looked weird. I think it had something to do with the user being added...

Screen Shot 2019-04-04 at 9 01 34 PM

Do you mind creating an issue for it. Any some steps you took. Would love to be able to replicate it and fix it. Thank you @jeffgolenski

@simison
Copy link
Member

simison commented Apr 5, 2019

When I landed at the checklist, I wanted to turn on monitoring first:

  • "Do it" brought me to the setting, I toggled it on
  • Then it said congrats or similar, and I pressed the ping button to get back to the checklist
  • The checklist was still suggesting to do the monitor step
  • So I did... ending up to settings where monitor is toggled already on:
    Screenshot 2019-04-05 at 11 30 07
  • So I returned to the checklist as prompted, and this time I could see the monitor checked:
    Screenshot 2019-04-05 at 11 30 20

@simison
Copy link
Member

simison commented Apr 5, 2019

I completed the last item on the checklist (single sign-in), and it jumped above the other un-finished items. Suppose this is expected, just thought to mention:

image

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Left a couple minor language notes and some UX observations.

Code looks good!

title={ translate( 'WordPress.com sign in' ) }
/>
{ isPaidPlan && productInstallStatus && (
<Task
Copy link
Member

Choose a reason for hiding this comment

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

As a future iteration, it might make sense to try get this into JETPACK_CHECKLIST_TASKS so that there's only one place where to look for these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call!

@tyxla
Copy link
Member Author

tyxla commented Apr 5, 2019

Do you mind creating an issue for it. Any some steps you took. Would love to be able to replicate it and fix it.

Sure thing @enejb - see #32060.

@tyxla
Copy link
Member Author

tyxla commented Apr 5, 2019

Thanks for the reviews and testing everyone 🙌

Going to address some minor suggestions by Mikael and then ship.

@tyxla
Copy link
Member Author

tyxla commented Apr 5, 2019

When I landed at the checklist, I wanted to turn on monitoring first:

  • "Do it" brought me to the setting, I toggled it on
  • Then it said congrats or similar, and I pressed the ping button to get back to the checklist
  • The checklist was still suggesting to do the monitor step
  • So I did... ending up to settings where monitor is toggled already on:
    Screenshot 2019-04-05 at 11 30 07
  • So I returned to the checklist as prompted, and this time I could see the monitor checked:
    Screenshot 2019-04-05 at 11 30 20

Yeah, good catch. I've seen that too, with other tasks - seems like there are some caching problems with the existing tasks - I'm going to look at resolving those in another PR.

@tyxla tyxla added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 5, 2019
@tyxla tyxla merged commit 003bdd3 into master Apr 5, 2019
@tyxla tyxla deleted the update/jetpack-checklist-dynamic-tasks branch April 5, 2019 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Checklist Jetpack NUX [Pri] Normal Schedule for the next available opportuinity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants