-
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
Home: Podcast Launched Card #47538
Home: Podcast Launched Card #47538
Conversation
count: currentTask.timing, | ||
args: [ currentTask.timing ], | ||
} ) } | ||
{ currentTask.timing && ( |
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.
The decision here is to make the CurrentTaskItem
more generic to adhere with designs.
@@ -52,7 +56,7 @@ const CurrentTaskItem = ( { currentTask, skipTask, startTask, useAccordionLayout | |||
className="site-setup-list__task-skip task__skip is-link" | |||
onClick={ () => skipTask() } | |||
> | |||
{ translate( 'Skip for now' ) } | |||
{ currentTask.isSkippableText || translate( 'Skip for now' ) } |
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.
Making this more generic as well so we can pass custom "Skip" text
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~115 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
2bfeb0a
to
872ab92
Compare
@krymson24 Looks like this is close – can you fill out the issue description and testing instructions please so we can push it out 🎉 Are there any other questions or blockers?.. |
872ab92
to
b416395
Compare
Shifting back to this now. I got caught up in all the GA4 urgency. |
Frontend is complete, but I still need to figure out how and where to send a list of tasks from the backend that correspond to the podcast-specific tasks. |
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.
Already gave feedback in D52925-code, but giving it here too as for closing the loop.
When I look at the design iterations (i1: pbAPfg-Vv-p2, i2: pbAPfg-Z3-p2, i3: pbAPfg-ZK-p2, i4: pbAPfg-14r-p2), I don't see any discussion around the site setup process in My Home, so I have a feeling that the mockup was only added to denote there is a site setup in place that we could customize, but no requirements have been defined.
Can we confirm this? I think the current site setup works perfectly for podcasting sites as well, so we may not need to customize it (or at least not for a v1 release):
If from a product perspective we are interested in tweaking the site setup tasks, then I'd suggest to keep using the same My Home view and card (VIEW_SITE_SETUP
and TASK_SITE_SETUP_CHECKLIST
) and apply the needed modifications in the getTask
function (client/my-sites/customer-home/cards/tasks/site-setup-list/get-task.js
). The benefit of this approach is that we'll keep using the logic we already have in the server for the current My Home view (such as displaying it only if there is a pending task).
For cases where tasks don't need to change its behavior but only its wording (like the "Welcome to your podcast site!" task, which replicates the behavior of the existing "Your site has been created!" task), then we can use the new selector introduced in #47747: wp-calypso/client/my-sites/customer-home/cards/tasks/site-setup-list/get-task.js Lines 76 to 92 in 80f37eb
case CHECKLIST_KNOWN_TASKS.START_SITE_SETUP:
const isPodcastingSite = !! getAnchorPodcast( siteId );
taskData = {
timing: 1,
label: isPodcastingSite ? translate( 'Continue building your site' ) : translate( 'Site created' ),
title: isPodcastingSite ? translate( 'Welcome to your podcast site!' ) : translate( 'Your site has been created!' ),
description: isPodcastingSite
? translate( "Now that you've created your site, we'll guide you through completing a few additional steps to finish building your site.")
: translate( "Next, we'll guide you through setting up and launching your site." ),
actionText: translate( 'Get started' ),
...( ! task.isCompleted && {
actionDispatch: requestSiteChecklistTaskUpdate,
actionDispatchArgs: [ siteId, task.id ],
} ),
actionAdvanceToNext: true,
completeOnView: true,
};
break; That way we don't need to introduce any changes in the back-end. |
Seems this is the case according to 322-gh-Automattic/dotcom-manage#issuecomment-733805456 ?
|
@sfougnier see screenshot in the PR description. Is having the |
Removed the reliance on the backend. @mmtr based on my comment here: #47747 (comment), I used the code for the selector you created for the logic to determine if the site is a podcasting site. |
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.
I may be misunderstanding @sfougnier's comment:
the task copy match what is in production today
But I think we don't want to change the copy of the initial task and thus this PR can be closed?
), | ||
isSkippable: true, | ||
isSkippableText: translate( 'Dismiss' ), | ||
actionText: translate( 'Continue' ), |
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.
I believe this is missing the actionDispatch
, actionDispatchArgs
, actionAdvanceToNext
and completeOnView
properties responsible for moving to the next site setup task.
Clarified in p1606490101041200-slack-C0Q664T29?thread_ts=1606343379.020400&cid=C0Q664T29 by @kwight:
|
cf879a5
to
43778da
Compare
Alright, I simplified the PR by simply overriding the task title on podcasting sites, since the rest of the task copy should match what we have in production. |
👍 Can we also update any Tracks events, keeping the same event name but adding a |
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.
Wonderful, works great for me 🎉
@sfougnier based on the screenshots, can you confirm this is what we want? |
I must have missed the ping for this. Can we remove the completed label for the podcast scenario? It's a little confusing. Otherwise, everything looks good. |
@@ -14,11 +14,12 @@ const CurrentTaskItem = ( { currentTask, skipTask, startTask, useAccordionLayout | |||
return ( | |||
<div className="site-setup-list__task task"> | |||
<div className="site-setup-list__task-text task__text"> | |||
{ currentTask.isCompleted ? ( | |||
{ currentTask.isCompleted && ! currentTask.hideLabel && ( |
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.
I added another prop here so that we preserve the true isCompleted
value for recordTracksEvent
. I was hoping to change the isCompleted
prop right as it gets passed to CurrentTaskItem
, but it looks like these objects are immutable, so this was the best alternative.
@@ -89,6 +90,12 @@ export const getTask = ( | |||
actionAdvanceToNext: true, |
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 you expand the code above, you'll notice a timing
prop for this checklist task. The text for this task is "Your site has been created". This implies that the task is complete, so I don't see why we would ever even show "timing" per the previous or existing logic in client/my-sites/customer-home/cards/tasks/site-setup-list/current-task-item.jsx
. We could probably remove it to avoid confusion.
@sfougnier done, thank you! |
f888821
to
75e8d79
Compare
Thanks for your changes, @krymson24! |
Changes proposed in this Pull Request
These changes add a primary card specific to the onboarding experience of a user who has just created a podcast website.
Testing instructions
wp shell --user=<YOUR_WPCOM_USERNAME> --url=<YOUR_SITE_ADDRESS>
.update_blog_option( <SITE_ID>, 'anchor_podcast', '3e51070' );
.is_podcasting_site
event prop:anchor_podcast
option value from your site:wp shell --user=<YOUR_WPCOM_USERNAME> --url=<YOUR_SITE_ADDRESS>
.delete_blog_option( <SITE_ID>, 'anchor_podcast' );
.Fixes 322-gh-dotcom-manage