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: rollout to other goal types. #26721

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

taggon
Copy link
Contributor

@taggon taggon commented Aug 16, 2018

The checklist is currently available only for a blog type site. This PR will roll out it to other goal types so our customers have a more unified experience.

How to test

  1. Create a new site with any other type but store.
  2. See if the checklist shows up to you.
  3. For a non-blog site, "Publish your first blog post" or "You uploaded a profile picture" items should be omitted.

@taggon taggon added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Checklist [Status] Needs e2e Testing labels Aug 16, 2018
@taggon taggon self-assigned this Aug 16, 2018
@matticbot
Copy link
Contributor

return null;
}

return get( site, 'options.design_type', '' );
Copy link
Member

Choose a reason for hiding this comment

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

You can leverage getSiteOption like this:

const siteDesignType = getSiteOption( state, siteId, [ 'design_type' ] );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove the file in favor of getSiteOption. Thanks. :)

@taggon taggon 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 Aug 16, 2018
@taggon taggon force-pushed the signup/rollout-chcklist-to-other-goal-types branch from 65f5113 to da7957a Compare August 17, 2018 13:48
@taggon
Copy link
Contributor Author

taggon commented Aug 17, 2018

Rebased

@fditrapani
Copy link
Contributor

I tested this with the other options and the checklist shows for me. The copy could use some work but we could leave that for another PR.

@taggon taggon force-pushed the signup/rollout-chcklist-to-other-goal-types branch from da7957a to 7de13a4 Compare August 21, 2018 13:57
@taggon taggon force-pushed the signup/rollout-chcklist-to-other-goal-types branch from 7de13a4 to 32d172d Compare August 23, 2018 12:00
@taggon
Copy link
Contributor Author

taggon commented Aug 23, 2018

Rebased

@fditrapani
Copy link
Contributor

Tested this again and it works as expected. 🎉

@mattwiebe mattwiebe self-requested a review August 23, 2018 20:15
Copy link
Contributor

@mattwiebe mattwiebe left a comment

Choose a reason for hiding this comment

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

I'd love to see some kind of consolidation of checking if the design type supports the checklist rather than the manual checks in several different places, but I don't consider that a blocker.

Otherwise this does what it's supposed to, looks good.

@taggon
Copy link
Contributor Author

taggon commented Aug 24, 2018

I'd love to see some kind of consolidation of checking if the design type supports the checklist rather than the manual checks in several different places, but I don't consider that a blocker.

I usually do code in that way too. :) But I leave them for now since we're going to roll out the checklist to all users, which means the type checking will be removed in the near future.

Thanks for your review! 👍

@taggon taggon merged commit e5612c7 into master Aug 24, 2018
@taggon taggon deleted the signup/rollout-chcklist-to-other-goal-types branch August 24, 2018 12:11
@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 Aug 24, 2018
@sirreal
Copy link
Member

sirreal commented Aug 24, 2018

This seems to have introduced the regression #26870 fixed by #26876.

@sirreal
Copy link
Member

sirreal commented Aug 25, 2018

This seems to have introduced the regression #26911 fixed by #26912

sirreal added a commit that referenced this pull request Sep 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.

5 participants