-
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
Copy Site: Prototype flow to copy an existing site #71913
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~5165 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~526 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. Async-loaded Components (~13197 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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. |
...nt/landing/stepper/declarative-flow/internals/steps-repository/automated-copy-site/index.tsx
Outdated
Show resolved
Hide resolved
client/landing/stepper/declarative-flow/internals/steps-repository/intro/index.tsx
Show resolved
Hide resolved
...nt/landing/stepper/declarative-flow/internals/steps-repository/automated-copy-site/index.tsx
Outdated
Show resolved
Hide resolved
...nt/landing/stepper/declarative-flow/internals/steps-repository/automated-copy-site/index.tsx
Outdated
Show resolved
Hide resolved
href={ copySiteHref } | ||
onClick={ () => recordTracks( 'calypso_sites_dashboard_site_action_copy_site' ) } | ||
> | ||
{ __( 'Create a copy of this 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.
{ __( 'Create a copy of this site' ) } | |
{ __( 'Copy site' ) } |
Can we shorten 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.
Done here 3306167
Don't you think that Copy site
is a vague description?
Users may be confused and think they will copy the site URL to the clipboard to paste it somewhere.
I suggest using Duplicate site
or Clone 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.
Don't you think that
Copy site
is a vague description?
Users may be confused and think they will copy the site URL to the clipboard to paste it somewhere.
That's a good point. We can iterate on it.
@@ -244,6 +274,7 @@ export const SitesEllipsisMenu = ( { | |||
<ManagePluginsItem { ...props } /> | |||
{ ! isNotAtomicJetpack( site ) && <HostingConfigItem { ...props } /> } | |||
{ site.is_coming_soon && <PreviewSiteModalItem { ...props } /> } | |||
{ isEnabled( 'sites/copy-site' ) && <CopySiteItem { ...props } /> } |
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.
Can we make sure this is only present for the site owner too?
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 check is inside the component.
wp-calypso/client/sites-dashboard/components/sites-ellipsis-menu.tsx
Lines 190 to 192 in 24545be
if ( ! hasAtomicFeature || ! isSiteOwner || ! plan ) { | |
return null; | |
} |
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.
Ah, cool. I think it'd be easier to read if it was outside of the component, but not a strong opinion on my end.
* CopySite: Auto-add used plan to the cart and trigger the site copy-transfer * CopySite: increase the progress bar flow
* fix: Remove adding product to cart before submit to avoid race conditions of dispatch and window.assign * fix: Move setting the cart item inside the submit to avoid console errors * add: Refactor automated copy site component and use a progress bar to show the copy of the site
…te` feature flag (#72056) * CopySite: use hyphens in step urls * CopySite: reduce the text in Copy Site SMP CTA Action * CopySite: fix siteDetails type * CopySite: add guard to make sourceSlug a required param * CopySite: enable flow only if feature flag sites/copy-site is enabled * CopySite: restore global.scss unwanted changes
757cd5b
to
40d2a0d
Compare
* fix: Remove product_slug from copy site parameters * fix: Rename dispatch to use useReduxDispatch
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.
Looks great, and works as expected. Good job on adding a new onboarding flow 🙂
title: __( 'Copy Site' ), | ||
text: createInterpolateElement( | ||
__( | ||
'You’re 5 minutes away from<br />creating a new copy site from <SourceUrl/>.<br />Ready?' |
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.
Should we validate sourceSlug at this point? I can replace it with a simple site URL, and the page renders saying it will copy my simple site.
The flow will end up with the error as REST API doesn't allow for that, but it makes sense to validate it early.
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've added 1503-gh-Automattic/dotcom-forge for 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.
Seems to work fine 👍 Just a few small nits remaining
@@ -244,6 +274,7 @@ export const SitesEllipsisMenu = ( { | |||
<ManagePluginsItem { ...props } /> | |||
{ ! isNotAtomicJetpack( site ) && <HostingConfigItem { ...props } /> } | |||
{ site.is_coming_soon && <PreviewSiteModalItem { ...props } /> } | |||
{ isEnabled( 'sites/copy-site' ) && <CopySiteItem { ...props } /> } |
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.
Ah, cool. I think it'd be easier to read if it was outside of the component, but not a strong opinion on my end.
case 'processing': { | ||
const destination = addQueryArgs( `/setup/${ this.name }/automated-copy`, { | ||
sourceSlug: urlQueryParams.get( 'sourceSlug' ), | ||
siteSlug: providedDependencies?.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.
siteSlug
is a little ambiguous here. Should it be destinationSlug
?
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.
siteSlug
is used as a convention in some steps to refer to the site we are creating. However I find it confusing as well, and in this particular scenario I guess it makes sense to rename it to destinationSlug
.
I also would like the opinion of @sejas as 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.
Let's keep siteSlug
for now. That param name makes it easier to grab the site using the useSite
hook.
…72081) * fix: Change the theme to the default for WP Sites at copy site flow * update: Move default theme to a constant
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7732008 Hi @sejas, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
Translation for this Pull Request has now been finished. |
Proposed Changes
Copy Site
item on SMP site actions for atomic sites that you own.Copy Site
flow to create a new site, buy a plan and migrate it to Atomic.Testing Instructions
/sites
.Create a copy of this site
.Pre-merge Checklist