Skip to content
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

[WIP] Try to rethink Stepper and logins #91074

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

daledupreez
Copy link
Contributor

Related to #90680

Proposed Changes

  • This PR builds on the exploration in Stepper Migration: Add user step to the hosting flow #90680 to see if we can pull login handling into the core Stepper logic in a way that requires minimal work from someone building a flow. There are a few elements to the exploration:
    • A flow needs to set the usesLocalLogin flag to opt in to the new handling
    • When opted in, the flow doesn't need to specify the user step in useSteps(), as the framework will add the user step as the first step if the current user isn't logged in. This also means the fallback/default logic for no step ('') will show the user step.
      • This logic will need to be relaxed/expanded to handle flows where some steps don't require a login, but we can solve for that later.
    • If we're rendering a step other than the user step, but we detect that the user isn't logged in and the flow is opted in to the new behaviour, we redirect to the user step.

Why are these changes being made?

  • This PR explores what it might look like for the Stepper to more fully abstract away some of the complications that arise from having the user/login step available to us.

Testing Instructions

  • Run this branch locally or via Calypso.live
  • Open a window where you are not logged in and navigate to /
  • Open the developer console and enter the following in the JS console to enable the new flow:
sessionStorage.setItem('flags', 'onboarding/user-on-stepper-hosting' );
  • Navigate to /setup/new-hosted-site-user-included
    • Verify that you're redirected to the user step
  • Navigate to /setup/new-hosted-site-user-included/
    • Verify that you're redirected to the user step
  • Navigate to /setup/new-hosted-site-user-included/plans (or any other step)
    • Verify that you're redirected to the user step
  • Log in from the user step
    • Verify that you're taken to the plans step (I haven't tested this - I wasn't sure how to trigger a login for an existing account)

We'll also want to triple-check that these changes have no impact on the standard/existing Stepper functionality. (They shouldn't but we should check!)

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@daledupreez daledupreez added [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. DO NOT MERGE [Feature Group] Signup & Site Onboarding Tools for user registration and onboarding new users to the site. Stepper labels May 23, 2024
@daledupreez daledupreez self-assigned this May 23, 2024
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 23, 2024
@matticbot
Copy link
Contributor

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

App Entrypoints (~3937 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
entry-stepper     +41379 B  (+1.9%)    +3937 B  (+0.7%)

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

Sections (~73456 bytes removed 📉 [gzipped])

name                                parsed_size            gzip_size
with-theme-assembler-flow              -40220 B  (-65.3%)    -5021 B  (-43.1%)
site-setup-flow                        -40220 B  (-58.2%)    -4985 B  (-36.6%)
assembler-first-flow                   -40220 B  (-55.2%)    -5053 B  (-33.2%)
ai-assembler-flow                      -40220 B  (-55.0%)    -5101 B  (-33.1%)
update-design-flow                     -40194 B   (-3.4%)    -5456 B   (-1.6%)
update-options-flow                    -40181 B  (-96.1%)    -4990 B  (-87.5%)
trial-wooexpress-flow                  -40181 B  (-87.8%)    -4897 B  (-67.7%)
site-migration-flow                    -40181 B  (-77.4%)    -5009 B  (-55.0%)
migration-signup                       -40181 B  (-80.9%)    -5011 B  (-59.1%)
hosted-site-migration-flow             -40181 B  (-76.9%)    -5057 B  (-55.0%)
free-post-setup-flow                   -40181 B  (-97.2%)    -4947 B  (-89.2%)
free-flow                              -40181 B  (-75.1%)    -5089 B  (-52.7%)
entrepreneur-flow                      -40181 B  (-81.9%)    -5079 B  (-60.0%)
new-hosted-site-flow-user-included      -3989 B  (-56.1%)     -923 B  (-40.5%)
plugin-bundle-flow                      -1098 B   (-0.6%)     -311 B   (-0.5%)
copy-site-flow                          -1098 B   (-0.2%)     -295 B   (-0.1%)
write-flow                              -1085 B   (-0.2%)     -279 B   (-0.1%)
transferring-hosted-site-flow           -1085 B   (-2.3%)     -314 B   (-1.8%)
tailored-ecommerce-flow                 -1085 B   (-9.9%)     -309 B   (-9.0%)
start-writing-flow                      -1085 B   (-4.3%)     -341 B   (-5.3%)
reblogging-flow                         -1085 B  (-12.7%)     -336 B  (-11.9%)
newsletter-post-setup-flow              -1085 B   (-0.9%)     -324 B   (-0.8%)
newsletter-flow                         -1085 B   (-5.0%)     -337 B   (-5.5%)
new-hosted-site-flow                    -1085 B  (-13.8%)     -325 B  (-12.3%)
link-in-bio-tld-flow                    -1085 B   (-0.1%)     -298 B   (-0.1%)
link-in-bio-post-setup-flow             -1085 B   (-0.9%)     -322 B   (-0.8%)
link-in-bio-flow-domain                 -1085 B   (-3.8%)     -330 B   (-4.3%)
link-in-bio-flow                        -1085 B   (-5.3%)     -326 B   (-5.5%)
hundred-year-plan                       -1085 B   (-2.8%)     -325 B   (-2.9%)
google-transfer                         -1085 B  (-17.3%)     -338 B  (-15.3%)
domain-user-transfer-flow               -1085 B   (-0.4%)     -327 B   (-0.6%)
domain-transfer                         -1085 B  (-18.1%)     -341 B  (-15.8%)
design-first-flow                       -1085 B   (-4.2%)     -330 B   (-5.0%)
connect-domain                          -1085 B   (-3.8%)     -322 B   (-4.2%)
build-flow                              -1085 B   (-0.2%)     -300 B   (-0.1%)
sensei-flow                              -208 B   (-0.0%)      -48 B   (-0.0%)
videopress-flow                          -195 B   (-0.0%)      -40 B   (-0.0%)
podcasts-flow                            -195 B   (-0.0%)      -61 B   (-0.0%)
domains                                   +13 B   (+0.0%)      -43 B   (-0.0%)
checkout                                  +13 B   (+0.0%)       -5 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 (~54 bytes removed 📉 [gzipped])

name                                        parsed_size           gzip_size
async-load-calypso-my-sites-checkout-modal       -195 B  (-0.0%)      -54 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.

@ddc22
Copy link
Contributor

ddc22 commented May 24, 2024

Thanks for igniting this conversation and putting up this PR. I myself have been thinking deeply about how the logged in ness fits in within the model of the stepper framework. I had the following thoughts about this approach, LMK what you think.

This logic will need to be relaxed/expanded to handle flows where some steps don't require a login, but we can solve for that later.

I don't think we should deprioritise this requirement. Specially because there have been frequent use cases where the user step is delayed to improve conversions. And the approach taken in the PR will fundamentally limit this use case. However working with a more loosely coupled user step (The existing approach where the user is modelled as a step rather than a framework level flag) did already provide a quite simple way to cater to this use case.

Nevertheless, I do love the fact that this approach will just be a switch to enable login. Is there probably a compromise where we make login integration easy as easy as a flag but make it a framework level feature. This is an approach that I envisioned when working on the step.

We leave everything as it is regarding the Flow API. However the framework will now listened to the ultra special User step. And all steps beyond the User step will automatically enforce a logged in state before the step is loaded.

image

@gabrielcaires
Copy link
Contributor

gabrielcaires commented May 28, 2024

@daledupreez I think we can have as a baby step move the Login verification to the StepRoute (IMAGE 1) and use a flag on the step definition on each to describe if the step requires login. (IMAGE 2)

Flow describing a step is required

image

Add logic to verify if the user is logging

Untitled 6

I am trying to think how we can have more compositions and explore better other layers we have.
I think having the <FlowRender /> component owning this responsibility will make harder future evolutions.

What do you think?

@alshakero
Copy link
Member

I agree with @gabrielcaires! I think it's great info to have for the future, too, to know which steps require auth and which steps don't. I like the intentionality of that design.

It should also be easy to redirect the user back to the step that caused them to be forced to log in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE [Feature Group] Signup & Site Onboarding Tools for user registration and onboarding new users to the site. [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Stepper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants