-
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
Site Migration: Show option to get an assisted migration on the migration instructions screen #100653
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
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 |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~3 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~9 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. |
I am adding @fditrapani as reviewer to confirm the copies. |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/17276206 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 @andregardi 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.
I think we need to fix the link tracking on this one. I'll take over the PR while @andregardi is out this week, so I won't officially "Request Changes" in the review.
interface DifmActionrops { | ||
navigateToDoItForMe: () => void; | ||
} | ||
|
||
export const DifmAction: FC< DifmActionrops > = ( { navigateToDoItForMe } ) => { | ||
const translate = useTranslate(); |
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.
Typo: DifmActionrops
=> DifmActionProps
We've also had previous discussions about using just Props
in the name, though we're not exporting it here, so not a major issue on that front.
Also, why an interface
and not a strict type
? It doesn't feel like we should expect or allow any non-prop arguments.
interface DifmActionrops { | |
navigateToDoItForMe: () => void; | |
} | |
export const DifmAction: FC< DifmActionrops > = ( { navigateToDoItForMe } ) => { | |
const translate = useTranslate(); | |
interface DifmActionProps { | |
navigateToDoItForMe: () => void; | |
} | |
export const DifmAction: FC< DifmActionProps > = ( { navigateToDoItForMe } ) => { | |
const translate = useTranslate(); |
@@ -65,19 +66,18 @@ export const ProvisionStatus: FC< ProvisionStatusProps > = ( { status } ) => { | |||
// Error handler. | |||
if ( currentAction.status === 'error' ) { | |||
const contactClickHandler = () => { | |||
recordMigrationInstructionsLinkClick( 'error-contact-support' ); |
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 think we should keep the original event structure so we can differentiate between the two click locations. With the new code, we're going to get exactly the same Tracks event from both clicks, so we won't be able to tell them apart.
}; | ||
|
||
text = translate( | ||
'Sorry, we couldn’t finish setting up your site. {{link}}Please contact support{{/link}}.', | ||
'Sorry, there was a problem setting up your site. {{button}}Let us take it from here{{/button}}.', |
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.
In thinking about this more, I don't think we should send all users to DIFM if there is an error here. I think they should be allowed to choose between contacting support and requesting an assisted migration.
I also think we need clearer language to make it 100% clear what the user is going to request/do. So I think we need two options here:
- [Contact support]
- [Let us migrate your site]
@fditrapani, I started working on this and I realised that showing "Contact Support" and "Let us migrate your site" ends up being pretty unwieldy. I ended up with the following working layouts, but I'd love feedback on both the idea of showing both and the layout I'm exploring before I push my changes into the PR. You'll notice that I'm using parallel secondary buttons in the desktop layout, and switching to a vertical, center-aligned layout for mobile. The CTAs are also above the error text, which is likely to be correct in most situations. DesktopMobile |
Thanks for the ping! I think we can take a slightly different approach in that when this error happens. Instead of updating the messaging in the blue box, can we hide it and replace it with a notice like this: (Don't mind the spacing as I was working off your screenshots). The approach should work well on desktop + mobile. What do you think? |
dc690f5
to
3d9f63f
Compare
@fditrapani, I explored this a little bit today, and it looks like the Here are some updated screenshots from this PR: MobileI added some CSS for the spacing above the buttons and vertical spacing between the buttons, but this DesktopMobile explorationsI explored making the "Let us migrate..." item a link and a secondary button: LinkSecondary button - mobile and desktop |
Thanks for looking into this and sharing the screenshots. That's unfortunate about the spacing. If i had to pick between the two options, I think it would better to go with the link. The two buttons have different border radiuses and they look clunky side by side. You mentioned being able to adjust the spacing with local overrides. Is that a good call or could it lead to us having issues down the line if they change something in the component? If it's not a problem, I can suggest a few more tweaks to help improve the layout. |
@fditrapani, I don't think we should, to be honest. The implementation for the |
@fditrapani, I've now reported the spacing problems with the
@fditrapani, I think we need to be extremely clear about which library this banner is coming from. I've flagged some issues with the implementation of the If we really want to stick with that component, we can, but it means that we need to add a wrapper component in Calypso, layer on the alternative styles, and then ensure that we remove the wrapper component when the upstream fixes are available in Calypso. That feels like a lot of work to pull this component into this screen. Is there a way for us to flag issues with certain components from |
Thanks for creating that issue @daledupreez. I agree that we don't want to go down the route of creating extra wrapper components. With the fix happening for the core component can we stick with that one?
We're still lacking some process here but the approach you took is best. Feel free to bring me in on an issue like you created if you need mock ups or design input. |
@fditrapani, it's not clear when we'll actually pick up the version of Gutenberg with the fix, so it could be a while before the component itself looks good. That's part of why I wanted to flag it for the design team: the implementation doesn't match the Figma, and probably won't for some amount of time, so we need some way of flagging that it's currently not a great component for the notice + actions use case. That still leaves us with what to do for this error notice/case. I will split out the top right escape hatch into a separate PR so we can get that moving, and I'll work on implementing a component that mimics the layout you've shown for the error case. (I'll definitely include a note to mention that it should be replaced with the |
Thank you for doing this. I think you identified a weak point in our general system that I'll bring up with the team. Coincidentally, I came across this comment on another PR.
Looks like it's a known issue but isn't communicated well as you pointed out.
That makes sense. Thanks for being flexible here. |
Just wanted to bring a comment over from another issue. For the |
Closes #92868
Proposed Changes
We are adding a escape hatch on the DIY migration instruction to reach out to assisted migration flow.
This will help users having a hard time doing the migration themselves.
Testing Instructions
Testing the top right action button
I'll do it myself
link on to right of thesite-migration-how-to-migrate
step.Having trouble? Let us migrate your site
link. Click on it.Testing error message action button

We are also updating the error message.
Instead of contacting support, we adding a "Let us take it from here" that directs the user to assisted migration.
To make sure you will get an error message I suggest updating your local Calypso file:

client/landing/stepper/declarative-flow/internals/steps-repository/site-migration-instructions/provision-status/index.tsx
On line 67, force the error condition to true.
Pre-merge Checklist