Skip to content

Comments

Migrating import flow to StepContainerV2#101738

Merged
paulopmt1 merged 11 commits intotrunkfrom
update/step-container-v2-for-import-flow
Mar 28, 2025
Merged

Migrating import flow to StepContainerV2#101738
paulopmt1 merged 11 commits intotrunkfrom
update/step-container-v2-for-import-flow

Conversation

@paulopmt1
Copy link
Contributor

@paulopmt1 paulopmt1 commented Mar 21, 2025

Related to #101191

Proposed Changes

  • Added support to the new stepper flow v2 on import screen
Before After
image image
image image
image image

Testing Instructions

  • Go to the /setup/onboarding flow locally or use the feature flag ?flags=onboarding/step-container-v2-migration-flow in testing servers.
  • Advance until you're at the Goals screen
  • Select Import or migrate an existing site

This work is partial

To make our reviews easier, I split the work into another three extra steps:

@github-actions
Copy link

github-actions bot commented Mar 21, 2025

@paulopmt1 paulopmt1 linked an issue Mar 21, 2025 that may be closed by this pull request
@matticbot
Copy link
Contributor

matticbot commented Mar 21, 2025

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~25 bytes removed 📉 [gzipped])

Details
name                   parsed_size           gzip_size
entry-stepper               -127 B  (-0.0%)      -25 B  (-0.0%)
entry-main                   +50 B  (+0.0%)      -23 B  (-0.0%)
entry-login                  +50 B  (+0.0%)      -23 B  (-0.0%)
entry-subscriptions          -45 B  (-0.0%)      -43 B  (-0.0%)
entry-domains-landing        -45 B  (-0.0%)      -43 B  (-0.0%)
entry-browsehappy            -45 B  (-0.0%)      -43 B  (-0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~4459 bytes added 📈 [gzipped])

Details
name                      parsed_size           gzip_size
stepper-user-step              -196 B  (-0.1%)     -776 B  (-1.0%)
async-step-use-my-domain       -196 B  (-0.0%)     -539 B  (-0.3%)
checkout                        +39 B  (+0.0%)      +19 B  (+0.0%)

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 (~84 bytes added 📈 [gzipped])

Details
name                                             parsed_size           gzip_size
async-load-signup-steps-plans-theme-preselected        +39 B  (+0.0%)      +21 B  (+0.0%)
async-load-signup-steps-plans                          +39 B  (+0.0%)      +21 B  (+0.0%)
async-load-signup-steps-domains                        +39 B  (+0.0%)      +23 B  (+0.0%)
async-load-calypso-blocks-editor-checkout-modal        +39 B  (+0.0%)      +19 B  (+0.0%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

matticbot commented Mar 21, 2025

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/step-container-v2-for-import-flow on your sandbox.

@paulopmt1 paulopmt1 force-pushed the update/step-container-v2-for-import-flow branch from 68a8a93 to 4589ff5 Compare March 24, 2025 16:27
@paulopmt1 paulopmt1 marked this pull request as ready for review March 24, 2025 20:17
@paulopmt1 paulopmt1 requested a review from a team as a code owner March 24, 2025 20:17
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 24, 2025
Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two tiny but important comments.

@paulopmt1 paulopmt1 requested a review from a team March 24, 2025 20:42
@paulopmt1 paulopmt1 changed the title Create initial structure to stepper flow v2 on import screen Migrating import flow to StepContainerV2 Mar 24, 2025
@daledupreez daledupreez requested a review from a team March 25, 2025 10:15
Copy link
Contributor

@daledupreez daledupreez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers without context, please specify why they need to run it against a local dev instance -- it took me a while to work out why I wasn't seeing the changes in the calypso.live branch. TL;DR: the feature is currently gated by the onboarding/step-container-v2 feature flag. I was able to test this with that flag explicitly enabled.

Along with the comments in the page, is there a reason we aren't migrating any additional screens as part of this PR? We have a large number of steps in the screen that are not included at the moment, so the experience is going to be janky with a partial transition to the new steps.

return (
<>
<DocumentHead title={ translate( 'Let us migrate your site' ) } />
<Step.CenteredColumnLayout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We explicitly narrowed this layout as part of the recent Code Blue V0 work -- see #100861 and #101519.

I see that we can either aim for a width of 4 or 6. 6 has a max-width of 615px, and the 4 case is really narrow with a max-width of 404px -- see the screenshot below. The current layout has a max-width almost exactly in the middle of that at 502px. @fditrapani, could you weigh in on what would be good for our use case and/or the CenteredColumnLayout wrapper?

Screenshot 2025-03-26 at 11 15 15

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping @daledupreez. Unfortunately neither of these options looks great. The wide one is too wide and hinders readability and the other one is too short making it looked cramped. Can we go with the wider one and then set a max width (400px) to the text to help with the readability? Like this:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we go with the wider one and then set a max width (400px) to the text to help with the readability?
The current layout has a max-width almost exactly in the middle of that at 502px

I'd prefer not to use custom styles for these blocks since we're aiming to reuse the style of these blocks across these screens.

I added a new layout-5 with a middle size between layout 4 and layout 6. Here's how it looks:

image

How does that sound to you @fditrapani and @fcoveram?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great to me! thanks @paulopmt1

@daledupreez daledupreez requested a review from a team March 26, 2025 09:26
@paulopmt1 paulopmt1 requested a review from alshakero March 26, 2025 18:30
@paulopmt1 paulopmt1 force-pushed the update/step-container-v2-for-import-flow branch from e58450b to 4761deb Compare March 26, 2025 18:32
@paulopmt1
Copy link
Contributor Author

Thanks for your review @daledupreez!

it took me a while to work out why I wasn't seeing the changes in the calypso.live branch.

Sorry, I didn't make it explicit that it should be run locally. I updated the description. Thanks for clarifying it for others!

I think it would be much better if we could push this up to be a general rule as part of the step container rather than adding a special case for this screen

That's fair. I used another strategy here.

is there a reason we aren't migrating any additional screens as part of this PR? We have a large number of steps in the screen that are not included at the moment

Good question! During development, I noticed that this would quickly grow, so I split the work into another three extra steps:

Since we'll release all of this at once (currently behind that feature flag), it will be find delivering a few screens at time, as even with these changes, production will remain the same.

@vykes-mac
Copy link
Contributor

vykes-mac commented Mar 27, 2025

Everything works good and is looking good apart from some minor deviations. (non-blocking)

  1. There seem to be more space from the top bar to the header title in the Prod version (right) than step container version on some of the pages.
  2. Subtitle have a different font colour on prod vs step container
  3. The video shows how the loader behaviour differs on both.
  4. Mobile version header and subtitle is centred on prod but doesn't seem that way on step version.

Nevermind 2 & 4, After looking at the figma design I realise the header and subtitle are left align on mobile and subtitle color is different.

image

image

image

image

image

image

Screen.Recording.2025-03-26.at.7.54.28.PM.mov

@autumnfjeld
Copy link
Contributor

Mobile version header and subtitle is centred on prod but doesn't seem that way on step version.

@vykes-mac This is per the Figma specs W9xI27S6Swvw5Ku21EbZvn-fi-9837_29100

image

@autumnfjeld
Copy link
Contributor

Functionality tested. ✅

Just want to make sure the rest of the import work is manageable to get done by March 31. If not maybe we treat it as a separate release? If it is manageable , then no worries. :)

@daledupreez
Copy link
Contributor

@autumnfjeld (and anyone else on Quake), can @Automattic/serenity help with converting more of the import and migration flows?

@paulopmt1
Copy link
Contributor Author

Just want to make sure the rest of the import work is manageable to get done by March 31.

Yeah, sending the other three screens by March 31 will be possible.

  1. There seem to be more space from the top bar to the header title in the Prod version (right) than step container version on some of the pages. Subtitle have a different font colour on prod vs step container
  2. Subtitle have a different font colour on prod vs step container
  3. Mobile version header and subtitle is centred on prod but doesn't seem that way on step version.

These changes are expected since the new StepContainerV2 implement them differently.

The video shows how the loader behaviour differs on both.

Thanks for pointing it out! We plan to fix that loading state as part of this task. Since it's only occurring behind the flag, and we'll fix it before March 31, we should be good to send it now.

@autumnfjeld
Copy link
Contributor

@autumnfjeld (and anyone else on Quake), can @Automattic/serenity help with converting more of the import and migration flows?

That would be fantastic @daledupreez .

@autumnfjeld
Copy link
Contributor

@Automattic/quake Let's keep these import PRs separate from the main flow release. See slack p1743109741691399/1743055172.691829-slack-C04H4NY6STW

Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now! Note that I only reviewed the code. I'm swamped right now and it will be a bit hard to test for me.

@paulopmt1 paulopmt1 force-pushed the update/step-container-v2-for-import-flow branch from 4761deb to ab83a2c Compare March 28, 2025 13:15
@paulopmt1
Copy link
Contributor Author

Let's keep these import PRs separate from the main flow release.

Updated it here to only work with the onboarding/step-container-v2-migration-flow feature flag.

@paulopmt1 paulopmt1 merged commit b991562 into trunk Mar 28, 2025
13 checks passed
@paulopmt1 paulopmt1 deleted the update/step-container-v2-for-import-flow branch March 28, 2025 18:14
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 28, 2025
"onboarding/interval-dropdown": true,
"onboarding/playground": true,
"onboarding/step-container-v2": true,
"onboarding/step-container-v2-migration-flow": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for separating this so we can focus on shipping well tested quality work for the main flow first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Onboarding style consistency: Import screen

9 participants