Start User: Add Continue as user to the sign in flow #58221

enejb · 2021-11-18T02:09:34Z

Instead of showing a notice that is not very helpful when the user clicks the browser back button on the domain screen.

Let's show the user a Continue as User component instead.

Changes proposed in this Pull Request

When the "user has completed the registration step" has been detected. We redirect the user to the registration form (same url) but with the ?user_completed=1 query parameter in the URL that helps us know if a user really want to be on this page. Usually, we skip the user registration step if they are already logged in.

Testing instructions

See

Screen.Recording.2021-11-17.at.6.10.26.PM.mp4

Visit
/start
Create a new user.
Once you are on the domain Screen. Use the browser back button.

Notice that you are logged in as a newly created user.

Click continue - you should end up on the domain screen again
Click the browser back button.
Click Login in as someone else

notice that you end up on the /start/user screen.
You are now logged out.

Other things to test
visit /log-in and /log-in/new notice that it looks as expected.

Related to #57287

github-actions · 2021-11-18T02:09:56Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-20992
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-20992&env=jetpack

matticbot · 2021-11-18T02:10:41Z

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

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

name         parsed_size           gzip_size
entry-login      +1189 B  (+0.1%)    +1610 B  (+0.6%)
entry-main        +100 B  (+0.0%)      +51 B  (+0.0%)

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

Sections (~999 bytes added 📈 [gzipped])

name             parsed_size           gzip_size
accept-invite        +2932 B  (+1.0%)    +1575 B  (+1.9%)
jetpack-connect      +1741 B  (+0.2%)    +1232 B  (+0.6%)
signup                -674 B  (-0.2%)      +95 B  (+0.1%)

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

name                          parsed_size           gzip_size
async-load-signup-steps-user      +3232 B  (+1.7%)    +1633 B  (+2.9%)
async-load-design-blocks           +898 B  (+0.0%)     +134 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.

annchichi · 2021-11-18T10:27:04Z

Hey @enejb in the video 0:27, just want to double-check here:

it will take the user back to this screen then auto direct to the "Let's Get Started - Continue" screen. Or we can just take the user directly to the "Let's Get Started - Continue" screen without showing the form in 0:27?
Let's get started copy should be centered.
Let's get started copy and Avatar spacing should be about 48px. Current one is about 30px.

If I spot anything, will let you know. Thank you! 😺

enejb · 2021-11-19T05:50:07Z

Yes I am hoping to remove the screen that shows up on 0:27.

I still need to play around a bit to try to figure that part out. @tyxla do you have any suggestions on how to fix that?

Let's get started copy should be centred.
I mostly keep the heading not centred mostly since on mobile most of the other screens (domains). Plans is centered agina.

I think we can probably fix that across all the screens on mobile. Would that make sense to you? @annchichi

Let's get started copy and Avatar spacing should be about 48px. Current one is about 30px.

Good catch!

tyxla · 2021-11-19T11:47:41Z

client/signup/steps/user/index.jsx

@@ -447,6 +451,17 @@ export class UserStep extends Component {
 isSocialSignupEnabled = true;
 }

+ if ( this.props.step && 'completed' === this.props.step.status ) {


Rendering doesn't seem like the right time to perform such a side effect. At the time the redirect happens, the redirect will have finished, which is likely why you're seeing the form shortly before the redirect.

Have you tried moving the redirection logic to componentDidUpdate()? Furthermore, it might need to be moved above to the controller level, since it happening in the step component could already be "too late".

Instead of showing a notice this lets is not very helpful Let's show the user a Continue as User component.

This is done so that we don't accidentally send the user to the continue page when they are creating a new user account

On second try in Firefox the back button didn't work any more. this fixes it by making sure that we don't remove the sign in step if there the page as loaded via history.

enejb · 2021-11-25T14:20:14Z

@tyxla I managed to get things working in FF now. 🥳
Here is a short video.

Screen.Recording.2021-11-25.at.6.14.04.AM.-.works.in.FF.mp4

Can you take a look again?

I also came across an error in the RegisterDomainStep

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
RegisterDomainStep@app:https:///./components/domains/register-domain-step/index.jsx:160:5
_class@app:https:///../packages/i18n-calypso/src/localize.js:36:9
ShoppingCartWrapper@app:https:///../packages/shopping-cart/src/with-shopping-cart.tsx:15:41
CartKeyWrapper@app:https:///./my-sites/checkout/with-cart-key.tsx:13:78
ConnectFunction@app:https:///../node_modules/react-redux/es/components/connectAdvanced.js:233:68
ShoppingCartProvider@app:https:///../packages/shopping-cart/src/shopping-cart-provider.tsx:18:30
CalypsoShoppingCartProvider@app:https:///./my-sites/checkout/calypso-shopping-cart-provider.tsx:20:37
div

That I think we should address in a different PR.

annchichi · 2021-11-26T08:48:27Z

Woohoo! 0:44 is working now. Thanks @enejb 🥳

Regarding the copy "Log in with another account", it was not clear if users can also sign out based on design feedback. So I think we can be even more clearer like this "Sign out or log in with another account" What do you think? 😄

Thanks for this!

enejb · 2021-11-26T21:24:07Z

It is definitely something we can update. I think a more refined wording makes sense here.

tyxla

This appears to be testing well!

I've left some feedback, and not sure if I can add much more from a framework perspective.

Could we perhaps ask someone more familiar with signup and onboarding, like @Automattic/ganon to provide a more thorough review?

tyxla · 2021-11-29T13:05:58Z

client/blocks/login/continue-as-user.scss

 color: var( --color-text-subtle );
 font-size: $font-body-small;
 text-align: center;
+ height: calc( 70vh - 80px );


This seems a bit made-up, can we add a comment as to where it came from?

You are right. I am not really sure exactly why those numbers have been chosen but they make things look okay across different screen sizes that I have tested.

client/blocks/login/continue-as-user.scss

client/blocks/signup-form/index.jsx

tyxla · 2021-11-29T13:09:18Z

client/signup/config/flows.js

+ // Remove the user step unless the user has just completed the step
+ // and then clicked the back button.
+ if ( ! param && ! detectHistoryNavigation.loadedViaHistory() ) {
+ flow = removeUserStepFromFlow( flow );


Can we confirm that this won't break something for an existing signup flow?

flow = removeUserStepFromFlow( flow ); was there before.
It used to be always applied. To test that it still works as expected.
I create the user and then just visit /start/user it should then redirect me to => /start/domains

I am not sure how else to test this?

There is also flows like p2 and p2-new where the user step isn't the first step. I briefly tried them and the step exclusion still seems to work as expected.

client/signup/steps/user/index.jsx

Co-authored-by: Marin Atanasov <[email protected]>

This makes it so that iPhone 5 screen size doesn't have to scroll

a8ci18n · 2021-11-30T01:22:01Z

client/blocks/login/continue-as-user.jsx

@@ -50,41 +56,60 @@ function ContinueAsUser( { currentUser, redirectUrlFromQuery, onChangeAccount }
 // like that, but it is better than the alternative, and in practice it should happen quicker than
 // the user can notice.

+ const notYouText = isSignUpFlow
+ ? translate( 'Not you?{{br/}} Sign out or log in with {{link}}another account{{/link}}', {


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 27 times:
translate( 'Not you?{{br/}}Log in with {{link}}another account{{/link}}' ) ES Score: 6

tyxla

I don't have much to add regarding code here 👍 This looks like solid work.

I'd defer the final review to someone from @Automattic/ganon as it falls under the signup / onboarding experience.

tyxla · 2021-11-30T08:46:11Z

client/blocks/login/continue-as-user.jsx

+ ? translate( 'Not you?{{br/}} Sign out or log in with {{link}}another account{{/link}}', {
+ components: {
+ br: <br />,
+ link: (


Should we declare this component separately? It seems to be duplicated across the 2 strings.

Great catch!

client/blocks/signup-form/index.jsx

Co-authored-by: Marin Atanasov <[email protected]>

p-jackson

I wasn't able to reproduce the issue seen in #57287. Following the login email links I never found myself on a user step with already filled in details.

I've done some testing and it seems to be working as designed. I am finding it quite confusing and tricky to reason about though, especially the use of the detectHistoryNavigation.loadedViaHistory() helper. Does this mean there's an intentional difference between the behaviour of using the browser back button and the back button in the framework UI? This seems quite confusing to me.

You may have already investigated this, but can the .status === 'completed' flag be used instead of adding a new ?user_completed query param? We're not always good at propagating query parameters between steps, so it might be better to rely on the data in the redux store.

p-jackson · 2021-11-30T22:26:20Z

client/signup/steps/user/index.jsx

@@ -367,6 +385,10 @@ export class UserStep extends Component {
 return this.props.step && 'completed' === this.props.step.status;
 }

+ userCreationCompletedWithAndHasHistory( props ) {


Would this be clearer without the With?
Am I interpreting this identifier correctly if I insert a pause like this? "user creation completed, and has history"

p-jackson · 2021-11-30T22:36:21Z

client/signup/config/flows.js

+ // Remove the user step unless the user has just completed the step
+ // and then clicked the back button.
+ if ( ! param && ! detectHistoryNavigation.loadedViaHistory() ) {
+ flow = removeUserStepFromFlow( flow );


There is also flows like p2 and p2-new where the user step isn't the first step. I briefly tried them and the step exclusion still seems to work as expected.

p-jackson · 2021-11-30T22:46:30Z

client/signup/steps/user/index.jsx

@@ -135,6 +136,19 @@ export class UserStep extends Component {
 }
 }

+ componentDidUpdate() {


Should this be in componentDidMount instead of on each prop update? Feels like we'd only want to potentially redirect on first mount.

I tried that and it didn't quite work as expected for me.
It seems that in some cases the component is already mounted and this part gets skipped.

…alypso into fix/start-user-logged-in

We don't need this condition any more since componentDidMount did the trick

This reverts commit ead51c7.

enejb · 2021-12-01T20:53:07Z

Thanks for the review and testing @p-jackson and @tyxla

I think this PR is ready to go.

I've done some testing and it seems to be working as designed. I am finding it quite confusing and tricky to reason about though, especially the use of the detectHistoryNavigation.loadedViaHistory() helper. Does this mean there's an intentional difference between the behaviour of using the browser back button and the back button in the framework UI? This seems quite confusing to me.

We removed the back button from the domain screen on mobile since users are pretty used to using the browser back button. Testing the framework UI back button on the desktop eventually leads me to wordpress.com/log-in page. Which doesn't seem to work as intended. Maybe we can address that in a future PR 🤔

This PR tries to fix this experience for mobile, by sending a user the screen where it is clear that they are already logged in.

The reason why we need redirect in js is that in the current flow, even if we just created a new user the browser doesn't have the full awareness that the user is logged in yet.
Which we get once the page is refreshed but once we refresh the page (redirect to itself) we lose the "user just completed registration" state that was in the redux store. (Which doesn't exist any more so having the state in the url via the query.

If the user is logged in already and they try the /start flow it is expected that they will be redirected to the domains page instead of the user registration page.

Hopefully the above makes sense 😅

p-jackson · 2021-12-01T22:21:11Z

The reason why we need redirect in js is that in the current flow, even if we just created a new user the browser doesn't have the full awareness that the user is logged in yet.

That does make sense now thank you. The signup framework is unique in how it's able to deal with the semi-logged state of the user. It's not surprising that the "Continue As User" screen only works after a full page refresh.

I haven't found any issues with this change in my testing, so no reason to hold off merging due to known bugs; it all seems to be working well. I am worried about the extra complexity that's being added to achieve this experience, but I'm not in a good position to judge the importance of this change to the mobile experience. Feel free to merge if this is going to be better for mobile web users.

a8ci18n · 2021-12-03T20:17:11Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7017513

Hi @enejb, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include this string: Not you?{{br/}} Sign out or log in with {{link}}another account{{/link}}

Thank you in advance!

a8ci18n · 2021-12-09T10:29:04Z

Translation for this Pull Request has now been finished.

* Start User: Add Continue as user to the sign in flow Instead of showing a notice this lets is not very helpful Let's show the user a Continue as User component. * Bump the spacing to 48px between the heading and text * Center the Heading on mobile on sign up * Only redirect if the user has used History Navigation This is done so that we don't accidentally send the user to the continue page when they are creating a new user account * Fix firefox issue with the back button On second try in Firefox the back button didn't work any more. this fixes it by making sure that we don't remove the sign in step if there the page as loaded via history. * Simplfy optional chaining Co-authored-by: Marin Atanasov <[email protected]> * Fix lint error * removed not needed function return * Update variable code style * Add handleOnChangeAccount function + track click * Continue as user Sign out text variation * Improve the visibility on iPhone 5 This makes it so that iPhone 5 screen size doesn't have to scroll * Add comment to explain the rational of the magic numbers. * remove the redundent attribute. Co-authored-by: Marin Atanasov <[email protected]> * DRY translation components * remove userCreationCompletedWithAndHasHistory since We don't need this condition any more since componentDidMount did the trick * Revert "remove userCreationCompletedWithAndHasHistory since" This reverts commit ead51c7. * Update the method name to remove With Co-authored-by: Marin Atanasov <[email protected]>

enejb requested review from aneeshd16, tyxla and donlair November 18, 2021 02:09

enejb self-assigned this Nov 18, 2021

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 18, 2021

tyxla reviewed Nov 19, 2021

View reviewed changes

Start User: Add Continue as user to the sign in flow

e3bfec8

Instead of showing a notice this lets is not very helpful Let's show the user a Continue as User component.

enejb force-pushed the fix/start-user-logged-in branch from 3d7c8e4 to e3bfec8 Compare November 24, 2021 23:15

enejb added 4 commits November 25, 2021 03:33

Bump the spacing to 48px between the heading and text

d79c16e

Center the Heading on mobile on sign up

2c1db02

Only redirect if the user has used History Navigation

99295a1

This is done so that we don't accidentally send the user to the continue page when they are creating a new user account

Fix firefox issue with the back button

a5ef787

On second try in Firefox the back button didn't work any more. this fixes it by making sure that we don't remove the sign in step if there the page as loaded via history.

tyxla reviewed Nov 29, 2021

View reviewed changes

enejb and others added 9 commits November 29, 2021 15:15

Merge branch 'trunk' into fix/start-user-logged-in

51c0392

Simplfy optional chaining

75bf467

Co-authored-by: Marin Atanasov <[email protected]>

Fix lint error

c775dfe

removed not needed function return

10bf74a

Update variable code style

20e3be3

Add handleOnChangeAccount function + track click

4e1bad5

Continue as user Sign out text variation

45a1c57

Improve the visibility on iPhone 5

7bd85b9

This makes it so that iPhone 5 screen size doesn't have to scroll

Add comment to explain the rational of the magic numbers.

71ddc33

a8ci18n reviewed Nov 30, 2021

View reviewed changes

enejb requested review from a team November 30, 2021 04:41

tyxla approved these changes Nov 30, 2021

View reviewed changes

enejb and others added 2 commits November 30, 2021 06:13

remove the redundent attribute.

71507a9

Co-authored-by: Marin Atanasov <[email protected]>

DRY translation components

d0b1c92

p-jackson reviewed Nov 30, 2021

View reviewed changes

enejb added 4 commits November 30, 2021 16:27

Merge branch 'fix/start-user-logged-in' of github.com:Automattic/wp-c…

fadffb3

…alypso into fix/start-user-logged-in

remove userCreationCompletedWithAndHasHistory since

ead51c7

We don't need this condition any more since componentDidMount did the trick

Revert "remove userCreationCompletedWithAndHasHistory since"

f3183d7

This reverts commit ead51c7.

Update the method name to remove With

f4cdec7

enejb merged commit f553894 into trunk Dec 3, 2021

enejb deleted the fix/start-user-logged-in branch December 3, 2021 19:23

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 Dec 3, 2021

mpkelly mentioned this pull request Oct 6, 2022

[Bug]: When clicking Add new site sometimes the user is redirected to /start/user and must press continue, even if already signed in #68562

Open

jsnajdr mentioned this pull request Jun 19, 2024

SignupForm: remove legacy code for handling back button #91956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start User: Add Continue as user to the sign in flow #58221

Start User: Add Continue as user to the sign in flow #58221

enejb commented Nov 18, 2021 •

edited

Loading

github-actions bot commented Nov 18, 2021 •

edited by matticbot

Loading

matticbot commented Nov 18, 2021 •

edited

Loading

annchichi commented Nov 18, 2021

enejb commented Nov 19, 2021

tyxla Nov 19, 2021

enejb commented Nov 25, 2021

annchichi commented Nov 26, 2021

enejb commented Nov 26, 2021

tyxla left a comment

tyxla Nov 29, 2021

enejb Nov 30, 2021

tyxla Nov 29, 2021

enejb Nov 30, 2021

p-jackson Nov 30, 2021

a8ci18n Nov 30, 2021

tyxla left a comment

tyxla Nov 30, 2021

enejb Nov 30, 2021

p-jackson left a comment

p-jackson Nov 30, 2021

enejb Nov 30, 2021

p-jackson Nov 30, 2021

p-jackson Nov 30, 2021

enejb Dec 1, 2021

enejb commented Dec 1, 2021

p-jackson commented Dec 1, 2021

a8ci18n commented Dec 3, 2021

a8ci18n commented Dec 9, 2021

Start User: Add Continue as user to the sign in flow #58221

Start User: Add Continue as user to the sign in flow #58221

Conversation

enejb commented Nov 18, 2021 • edited Loading

Changes proposed in this Pull Request

Testing instructions

github-actions bot commented Nov 18, 2021 • edited by matticbot Loading

matticbot commented Nov 18, 2021 • edited Loading

annchichi commented Nov 18, 2021

enejb commented Nov 19, 2021

Choose a reason for hiding this comment

enejb commented Nov 25, 2021

annchichi commented Nov 26, 2021

enejb commented Nov 26, 2021

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-jackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enejb commented Dec 1, 2021

p-jackson commented Dec 1, 2021

a8ci18n commented Dec 3, 2021

a8ci18n commented Dec 9, 2021

enejb commented Nov 18, 2021 •

edited

Loading

github-actions bot commented Nov 18, 2021 •

edited by matticbot

Loading

matticbot commented Nov 18, 2021 •

edited

Loading