Conversation
|
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: June 4, 2019. |
|
Should we show the button on free plans as well? It seems odd, since there isn't much to set up at that point, you are just invited to purchase a plan? |
There will be some new stuff coming to the checklist sometime after 7.4 release for free sites as well; lazy loading images & CDN for example. |
|
We definitely want to display this for free. As @simison mentioned, more things are coming to the checklist soon! @sirreal I just testing this - I'm noticing a difference in styles. For paid plans the button is in the correct place, but but free it is not. Looks great on mobile. Can you just bump the
|
|
Should we address multiple primary color buttons on the same page as a follow up to this? |
simison
left a comment
There was a problem hiding this comment.
Code is looking good 👍
Just small placeholder change I'd like to see before merging.
There was a problem hiding this comment.
This one class was inconsistent with the rest.
3e89000 to
4c9859b
Compare
|
Rebased so the primary CTAs are no longer repeated (#12430).
There was a misnamed class in exactly 1 place, which was the class I'd based the changes on. I fixed this class name so the different plans should be consistent. This also highlighted a lot of what appear to be leftover classes from previous plans pages. I've removed many rules that appear to be unused after searching for references.
I've taken care of that 👍 Here are updated mobile/desktop screenshots for all plans
|
jeherve
left a comment
There was a problem hiding this comment.
This looks good, and tests well for me. I think this should be good to merge after a design review.
I mentioned the lack of tracking on that button below, but I don't think that's blocking here, given that we are missing tracking on other buttons in there. It could be added in a future PR I think.
| export default function ChecklistCta( { siteSlug } ) { | ||
| return ( | ||
| <div className="jp-landing__plan-features-header-checklist-cta-container"> | ||
| <Button primary href={ `https://wordpress.com/plans/my-plan/${ siteSlug }` }> |
There was a problem hiding this comment.
Do we want to add tracking to clicks on that button?
Thanks for bringing this up. Yes, we need be adding tracking to all new buttons that are added, every single time. If we don't know if / how our customers are using what we design and build then we're just wasting our time. Can we get a tracks event in there @sirreal? Thank ya! |
@jeffgolenski yes, but I'd prefer to add it in a follow up since this has the approvals already. Can we consider this design approved? |
jeffgolenski
left a comment
There was a problem hiding this comment.
Did another run through. Looks great!
|
Tracks here: #12449 |
In #12277 and #12429, we added a "checklist" component to the Jetpack dashboard, as well as a CTA that would lead you to the checklist in Calypso. We never really expanded on that checklist and its display in the React dashboard, but we now have a Recommendations component we use in Jetpack, since #18437. This commit removes the elements from the checklist that we never used, and updates the CTA link in "My Plan" to point to the new Recommendations, when those recommendations must be shown to site owners.
Add a checklist CTA to the My Plan header based on #11997 (comment)
This introduces yet another primary button to this view. #12430 removes the primary buttons from all of the feature cards in the view.
Screens
Expand for screens
Testing instructions:
/wp-admin/admin.php?page=jetpack#/my-planon a Jetpack siteProposed changelog entry for your changes:
none