-
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
Replace overview when site is with a pending migration #95497
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
); | ||
} | ||
|
||
return ( | ||
<div className="hosting-features"> | ||
<div className="hosting-features__hero"> | ||
<HostingHero> |
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.
Components were extracted so that we could reuse them.
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~160 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~1945 bytes removed 📉 [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 (~48 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. |
a18a2c4
to
2150032
Compare
I added the "[Status] String Freeze" because we have many new texts, so we'll delay a little to merge this anyway. |
@@ -473,6 +473,7 @@ | |||
margin-top: 0; | |||
margin-left: 0; | |||
margin-right: 0; | |||
margin-bottom: auto; |
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 Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/16927618 Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @renatho for including a screenshot in the description! This is really helpful for our translators. |
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 good and works as described! Thanks for splitting those components.
I've added one small non-blocker suggestion.
@renatho Could you add some additional context or screen shots of what these mean? It's a bit unclear to me.
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Translation for this Pull Request has now been finished. |
@renatho Thank, I missed those subtleties. I can create an issue for adding the status to the header. @fditrapani Can you confirm that we should disable the Hosting settings tab for pending / started migrations? |
Thanks for the ping. This looks good from the screenshots. We had some back and forth about this with Michael. I figured it made sense to keep it disabled so you can stay focused on starting your migration. I can see the other side of it that people might want to poke around so I can be convinced otherwise. What do you all think? |
if ( isFreePlan ) { | ||
// For the flows where the checkout is after the choice. | ||
switch ( migrationType ) { | ||
case 'diy': | ||
continueMigrationUrl = addQueryArgs( | ||
{ | ||
...baseQueryArgs, | ||
destination: 'upgrade', | ||
how: 'myself', | ||
}, | ||
'/setup/site-migration/site-migration-upgrade-plan' | ||
); | ||
break; | ||
case 'difm': | ||
continueMigrationUrl = addQueryArgs( | ||
{ | ||
...baseQueryArgs, | ||
destination: 'upgrade', | ||
how: 'difm', | ||
}, | ||
'/setup/site-migration/site-migration-upgrade-plan' | ||
); | ||
break; | ||
default: | ||
continueMigrationUrl = addQueryArgs( | ||
baseQueryArgs, | ||
'/setup/site-migration/site-migration-how-to-migrate' | ||
); | ||
} |
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.
Just a little concern, as we're only checking if the site is on a free plan or not, in case the site is on a Personal
or Premium
plan, will it send the user to the credentials or how-to-migrate step instead of the pre-checkout step?
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.
Hmm!! Good point, @Imran92! I was thinking that in our flows users wouldn't be able to pick a Personal or Premium plan and then try to migrate, but after you said, I tested it and I could do it through the /start
, buying the plan (Personal or Premium), select the intent to migrate and use the flow.
I'll update it to a different check.
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.
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.
@renatho can you update the unit tests 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.
Oops! Fixed here: e5d856e
I'd agree with this logic. Makes sense to keep them focused on migrating. I'll update the issue to include this. |
case 'diy': | ||
continueMigrationUrl = addQueryArgs( | ||
baseQueryArgs, | ||
'/setup/migration/migration-how-to-migrate' |
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.
According to the issue, in case of migration-pending-diy
sticker, the user should be taken to the site-migration-instructions
step. Looks like this here is taking us to the migration-how-to-migrate
step. In case it's not intentional, should we fix it?
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.
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'm seeing a weird issue, but not sure why it's happening.
I had a site in a pending state. In the overview, section worked as expected, showing me correct button and everything relevant.
Then I changed the sticker to 'migration-completed-*'. But even after refreshing and hard-refreshing the page section many times, I still see the the migration-pending overview page.
Screen.Recording.2024-10-24.at.8.42.34.PM.mov
But apart from that, LGTM. So I'm approving. Feel free to look into the issue either separately or here.
3b1a5e6
to
91bfc81
Compare
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.
LGTM 👍 Cache gets cleared after a couple of minutes
Since the parent has an overflow hidden and a padding bottom it wasn't being respected.
When we implement the started status, we can also use the MigrationOverview, and remove this conditional.
Similar to the other mocks in these tests.
91bfc81
to
2e90338
Compare
I just rebased to fix a conflict, so I'm doing a sanity test, and I'll wait for the tests and merge it. |
Related to #95379 and #95380
Proposed Changes
ref
ashosting-migration-overview
, so we can identify when the user is coming from this page.This PR Doesn't Include
@donnapep, do you think it's fine to deploy it as it is and implement these other parts in other PRs? I think one issue would be fine for both changes. I'll wait for your answer to then create the new issue, otherwise, I'll continue working on this one.
Why are these changes being made?
Testing Instructions
/setup/migration
./sites
./sites
and click on "Start your migration"./sites
./setup/hosted-site-migration
and make sure it works properly.Screenshots
Pre-merge Checklist