-
Notifications
You must be signed in to change notification settings - Fork 799
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
My Plan: Add Checklist CTA #12429
My Plan: Add Checklist CTA #12429
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is looking good 👍
Just small placeholder
change I'd like to see before merging.
@@ -86,7 +90,7 @@ class MyPlanHeader extends React.Component { | |||
alt={ __( 'Jetpack Premium Plan' ) } | |||
/> | |||
</div> | |||
<div className="jp-landing__plan-iconcard-current"> | |||
<div className="jp-landing__plan-card-current"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern here. Appears to be working well otherwise! I also couldn't find any other uses of those stale class names.
Feel free to ship after addressing the i18n question!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-plan
on a Jetpack siteProposed changelog entry for your changes:
none