-
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: Update migration instructions screen #88813
Site Migration: Update migration instructions screen #88813
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
@javierarce I added a video showing the animations and interactions but feel free to see the real screen. |
@javierarce The show/hide eyes icon is different from the layout because there is a recommendation to use the wordpress/icons library https://wordpress.github.io/gutenberg/?path=/story/icons-icon--library |
You are totally right, sorry about that, I'll update the icon in Figma. Regarding the implementation of the design, it looks great, but if we made two little adjustments to the heights of the button and the input field, they'd look better. Could we make the height of the input field 44px and the height of the button 28px? @michaelpick could you and Francisco review the copy of the instructions step (5yhaC6umexdQLlLpCrCyEe-fi-2095_9011)? I think I never showed them to you. Here's the text:
|
@javierarce size updated! I also prevented the component from automatically selecting the key when it is hidden. |
...ng/stepper/declarative-flow/internals/steps-repository/site-migration-instructions/index.tsx
Outdated
Show resolved
Hide resolved
Fantastic, much better, thanks! |
One last thing that I noted (but we can update this after we get the content review) is the use of italics. Since we are using quotes the italics are redundant. |
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 have some quick feedback from inspection.
...ng/stepper/declarative-flow/internals/steps-repository/site-migration-instructions/index.tsx
Outdated
Show resolved
Hide resolved
{ isSuccess && migrationKey && ( | ||
<li> | ||
{ translate( | ||
'Copy and paste the migration key below in the {{em}}“Migrate Guru Migration Key”{{/em}} field and click {{strong}}Migrate{{/strong}}.', |
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.
Question: are the label and button CTA translated in the Blog Vault/Migrate Guru UI?
We run the risk of translating these terms when they are not translated in the underlying screen.
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.
Good question!
I checked and saw they were not translating the CTAs, so I applied this change d22bc4d to protect then to be translated.
...ng/stepper/declarative-flow/internals/steps-repository/site-migration-instructions/index.tsx
Outdated
Show resolved
Hide resolved
b860201
to
658ae3d
Compare
@javierarce Sure! I removed the quotes here 9f9478c |
Currently, the main Migrate Guru CTA are not translatable, so we want to protect the button and fields names to be translated from our translation team.
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 |
{ | ||
components: { | ||
migrationKeyField: ( | ||
<DoNotTranslateIt value="Migrate Guru Migration key" as="em" /> |
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.
What's the reasoning behind adding a DoNotTranslateIt
component?
maybe adding the markup in the input string directly and passing the Migrate Guru Migration key
| Migrate
strings as an arg is enough?
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.
@andres-blanco
The idea is to express the intent the value should not be translated, especially considering we don't have any kind of automated test testing translations.
My concern is we can easily remove the args value just because the intent is not clear and we don't have a test suite testing translations.
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.
ok, then my only comment is that it would be good to remove the markup from the DoNotTranslateIt
element and keep it in the input string. That way is more readable, and also extendable (imagine if you want to do something like <strong><em>STRING</em></strong>
).
But it's just a nitpick, not a blocker. Go with your gut :)
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.
Good point; I tested, and we can still do it via {{strong}} {{migrationKeyField /}} {{/ strong }}.
but I still agree with you because it blocks the translators to freely move the formatting position.
So, I update here 17eefe3
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.
Overall, this is LGTM, though I do have some non-blocking feedback.
@@ -0,0 +1,22 @@ | |||
import { useQuery } from '@tanstack/react-query'; |
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.
Nit: the filename has a typo. -migraiton-
=> -migration-
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.
Fixed 694f4fc
return useQuery( { | ||
queryKey: [ 'site-migration-key', siteId ], | ||
queryFn: () => getMigrationKey( siteId! ), | ||
retry: false, | ||
enabled: !! siteId, | ||
select: ( data ) => ( { migrationKey: data?.migration_key } ), | ||
refetchOnWindowFocus: false, | ||
} ); |
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 specify staleTime
on the useQuery()
call to allow the data to be kept in memory for a little while before we try to refetch? (reference)
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 set 60 seconds as staleTime as we discussed on slack.
a8022e4
export const useSiteMigrationKey = ( siteId?: number ) => { | ||
return useQuery( { | ||
queryKey: [ 'site-migration-key', siteId ], | ||
queryFn: () => getMigrationKey( siteId! ), |
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.
Is the !
intentional? If so, what does it do?!?
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.
Yes, It is saying that typescript can assume the siteId will be available when the function is executed.
The enabled flag is already ensuring it.
The line above ensures that react-query only triggers the queryFn when the siteId is available.
enabled: !! siteId,
`` `
.once() | ||
.reply( 200, { migration_key: 'some-migration-key' } ); | ||
|
||
const { result } = renderHook( () => useSiteMigrationKey( 'some-site-id' ), { wrapper } ); |
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.
Nit: the argument expects the siteId argument to be numeric, so we should likely test it with a number (or allow a site slug to be specified in the types).
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.
Fixed here 67c80a1
We are with the typescript checking disabled on the test files on our pipeline😢 .
max-width: 456px; | ||
transition: all 0.2s; | ||
|
||
&--hidden { |
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.
Nit: I have a strong personal preference for avoiding &
as selector/text duplicator in CSS selectors because it makes it much harder to find the selector when debugging in an IDE.
While that may not seem like a common use case, every now and then we hit issues with CSS selectivity in Calypso as a result of duplicate selector names that are defined in different CSS bundles, and knowing where and how the rules were defined is relatively important.
Also, I find I need to think extra-hard to make sure I know what &
actually represents, especially within patch diffs where the wrapping selector may not be visible.
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.
Thanks for the context:
I removed b24b270 the usage of &
{ translate( | ||
'You will see a screen showing multiple hosting providers. Select {{em}}Automattic{{/em}} as the destination host.', | ||
'Go to the {{a}}Migrate Guru page on your source site{{/a}}, enter your email address, and click {{strong}}{{migrateButton /}}{{/strong}}.', | ||
{ | ||
components: { | ||
em: <em />, | ||
a: <a href={ getMigrateGuruPageURL( fromUrl ) } target="_blank" rel="noreferrer" />, | ||
migrateButton: <DoNotTranslateIt value="Migrate" />, | ||
strong: <strong />, | ||
}, | ||
} | ||
) } |
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 don't have a super-strong opinion on this, but I do think it would still be relatively easy and clear if we handle this with args and a comment. (We may need the comment anyway.) e.g.
{ translate( | |
'You will see a screen showing multiple hosting providers. Select {{em}}Automattic{{/em}} as the destination host.', | |
'Go to the {{a}}Migrate Guru page on your source site{{/a}}, enter your email address, and click {{strong}}{{migrateButton /}}{{/strong}}.', | |
{ | |
components: { | |
em: <em />, | |
a: <a href={ getMigrateGuruPageURL( fromUrl ) } target="_blank" rel="noreferrer" />, | |
migrateButton: <DoNotTranslateIt value="Migrate" />, | |
strong: <strong />, | |
}, | |
} | |
) } | |
{ translate( | |
'Go to the {{a}}Migrate Guru page on your source site{{/a}}, enter your email address, and click {{strong}}%(migrate)s{{/strong}}.', | |
{ | |
comment: 'Note that the migrate arg will always be "Migrate", as the Blog Vault UI is not translated', | |
args: { | |
migrate: 'Migrate', | |
}, | |
components: { | |
a: <a href={ getMigrateGuruPageURL( fromUrl ) } target="_blank" rel="noreferrer" />, | |
strong: <strong />, | |
}, | |
} | |
) } |
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 feedback I received was that the translators sometimes don't read the comments.
Regarding the args, I explained my concern here.
#88813 (comment)
Oh, I meant to remove the italics and leave the quotes. We don't italicize that type of text. I've pinged Francisco to do a content review of the text. If possible, let's wait for it before we merge this PR. |
Ok, here are the corrections (1e1T4v-g_4Lu6Kv2uCRIRYGr7KAlqlv0hxcpTm34FWbA-gdoc):
|
@javierarce @daledupreez Due to some github bug I only could see your suggestions after I merged. |
* Add hook to get the site-migration-key * Add component to show/hide the migration key * Update layout * Add animation * Adjust migration key input size * Replace Automattic to WordPress.com * Remove quotes * Add log when we are showing the migration key fallback instructions Improve copy * Improve copy * Fix test * Protect the CTA to be translated Currently, the main Migrate Guru CTA are not translatable, so we want to protect the button and fields names to be translated from our translation team. * Fix typescript checking * Fix typescript checking * Remove tag from DoNotTranslateIt * Add missing strong tag * Fix unexpected scroll
Related to #88588
Proposed Changes
More context: p1711029899474269-slack-C0Q664T29
Testing Instructions
SHORT VERSION:
/setup/site-migration/site-migration-instructions?from=[JN site]&siteSlug=[YOUR_WORDPRESS.COM site created by previous migrations]
LONG VERSION:
/start
URL using the calypso.live instructionssessionStorage.setItem('flags', "onboarding/new-migration-flow")
https://test-vanilla-wp.godaddy.test-wpcom-migrations.net/
NOTE: The screen variation with the migration key is only shown the first time the user sees the screen, after a reload only the second variation is shown.
NOTE 2: If you want to reset the site state to see screen 1 again, you just need to open the URL([TARGET_SITE]/_cli) and run the command
wp option delete wpcom_site_migration_migrate_guru_key_read
to delete the site option we are using to not show again the migration key.Screenshots
1a. Desktop - Migration key available
1.b Mobile - Migration key available
2a. Desktop - Migration key not available
2b. Mobile - Migration key not available
Interactions:
7itXHm.mp4