-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Plans: Add Checklist to My Plan page (JP) #25870
Conversation
4826c1d
to
bc92484
Compare
488799d
to
5619fb6
Compare
client/my-sites/checklist/main.jsx
Outdated
|
||
return ( | ||
<FormattedHeader | ||
headerText={ translate( 'Welcome back!' ) } |
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.
Hi! I've found a possible matching string that has already been translated 24 times:
translate( 'Welcome Back :)' )
ES Score: 15
See 1 additional suggestion in the PR translation status page
Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).
client/my-sites/checklist/main.jsx
Outdated
const { translate } = this.props; | ||
|
||
if ( displayMode === 'free' ) { | ||
return translate( 'Your site has been created!' ); |
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.
Hi! I've found a possible matching string that has already been translated 2 times:
translate( 'Your new site has been created!' )
ES Score: 10
Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).
This reverts commit 23b2fbe6ba6e6b6bcbbb461a3dd52d941993290e.
5619fb6
to
32dc24d
Compare
The checklist was showing for Atomic sites, I've just pushed a change to hide it for those sites. |
A note on this flow:
Step 2 lands on the plans page. Presumably the intention is to incite users to investigate plans offerings. By step 4 they've been whisked away through a series of steps. Now they don't see plans or the checklist added by this PR. cc: @joanrho |
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.
A few notes about functionality, likely none are introduced by this PR
- My first checklist item is for Jetpack Monitor. I click the task, enable the setting and return. I believe the settings API request hasn't completed and the checklist item remains unchecked. If I click again and return to the checklist, it is.
- Is "Automatic Plugin Updates" enabled? When I click that item, no help appears.
- The end of the checklist steps returns to
/checklist/:SITE_SLUG
rather than/plans/my-plan/:SITE_SLUG
. It would be nice to "return" to where you started from.
Feature flag works well 👍
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.
If the notes I mentioned aren't related to this PR, should be good to 🚢
Yeah, I was thinking of adding some Redux logic to reflect that change right after it's been made (or maybe even show a loading spinner while the network request hasn't returned yet, see #25935).
The corresponding Guided Tour hasn't been merged yet (#25474).
Ah yup, I forgot to add a note about that, sorry. Guided Tours right now hard-code where they finish, and right now, that points to Thanks for reviewing! Going to merge & file cards/issues so we don't forget about items 1 and 3. |
@ockham Don't we end all Guided Tours with Notices that link to "Go back to checklist"? |
Uh yeah -- I thought I'd just change the Jetpack specific ones to point to |
@ockham ah yes, that's exactly what we want to do, since /checklist doesn't have a home yet. Just confirming all GTs end in notices with the link back to My Plan page (our checklist). |
Cool! FWIW, I've filed a card on the project board for this, https://github.com/Automattic/wp-calypso/projects/70#card-11207580 |
Screenshot
Testing Instructions
http://calypso.localhost:3000/plans/my-plan/<jpSite>
http://calypso.localhost:3000/plans/my-plan/<wpcomSite>
http://calypso.localhost:3000/plans/my-plan/<jpSite>
Fixes #25871