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

SignupForm: remove legacy code for handling back button #91956

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Jun 19, 2024

The user step in signup has one problem that it needs to solve: if the user step is completed and the user account is created, you proceed to the next step, like domains. Now what happens when you click the back button? You go back to user. Without any treatment, this step would should the signup form again, confusing the user to fill and submit it again, creating a duplicate account. We want to prevent that. And over the years, we iterated through three solutions.

The first solution was in #27832 by @mattwiebe. It solved it by using an userCreationComplete helper to detect that the user step is completed and show a notice and a slightly altered form. The user is warned that the user is already created.

The second iteration was #58221 by @enejb and it showed a ContinueAsUser UI instead of the signup form when a back-button navigation was detected and the user was logged in. It implemented history navigation detection and ?user_completed=true query param to show the right thing. After this PR, the user should never see the first version with a notice again after clicking the back button.

And the final iteration, which obsoleted the previous two, was #65908 by @zaguiini and @danielbachhuber. It added a componentDidMount/Update effect that simply redirects to the next step (goToNextStep) if the user step is completed. If you navigate from, e.g., /start/domain back to /start/user, you are immediately redirected back to /start/domains (the next step). There's no longer a need for /start/user to render anything special.

The only case where /start/user still shows ContinueAsUser is when it's the only step in the flow, like in account. But there's no next step to go back from. After the user step completes, the signup flow ends, its state is reset and it navigates to the flow "destination".

My PR removes all the code that is legacy and no longer needed. The userCreationComplete checks, the detectHistoryNavigation usages. One of the consequences is that many of the user components no longer need the step prop. Which is a good thing because it's a data structure specific to the classic signup framework, and all these components are also used in other contexts: Jetpack Connect, Invite Accept, Stepper.

In a sense this is also a followup to #91883.

How to test:
First thing to note is that most user creations today are passwordless, and passwordless signup doesn't have the back button problem. Because the flow is interrupted after signup, and you continue it only after opening a link in email, in another browser tab.

So, first thing I did was to disable passwordless signup by disabling the signup/social-first feature flag in dev mode.

Then I went through the onboarding flow:

user-back.mov

Things that you should notice:

  • after the user creation is complete and before displaying the Domains step, there is a brief moment when just a white screen (with a little WP logo) is rendered. That's the step.status === 'completed' state where the User step renders nothing.
  • after you click back in the Domains step, you can see in the console (I put some logpoints in the Calypso router) that the app navigates back to /start/user but then redirects immediately back to /start/domains.

Another flow you should test is /start/account, because it's the special case where a flow has just a single user step.

@jsnajdr jsnajdr self-assigned this Jun 19, 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 Jun 19, 2024
@matticbot
Copy link
Contributor

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

Sections (~287 bytes removed 📉 [gzipped])

name                  parsed_size           gzip_size
jetpack-connect            -681 B  (-0.1%)     -152 B  (-0.0%)
accept-invite              -681 B  (-0.4%)     -132 B  (-0.3%)
update-design-flow         -147 B  (-0.0%)      -66 B  (-0.0%)
signup                     -147 B  (-0.1%)      -66 B  (-0.1%)
link-in-bio-tld-flow       -147 B  (-0.0%)      -66 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 (~253 bytes removed 📉 [gzipped])

name                          parsed_size           gzip_size
async-load-signup-steps-user      -1072 B  (-0.6%)     -253 B  (-0.5%)

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.

if (
prevProps.step?.status !== this.props.step?.status &&
this.props.step?.status === 'completed'
) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This change fixes a bug where the navigation from /start/user to the next step (start/domain) would be triggered twice, because the component happens to be rerendered twice in the completed state. Then you end up with /start/domain being twice in the window history, and you'll be confused when clicking the back button. You have to click twice.

This updated code is equivalent to how useEffect would work:

useEffect( () => {
  if ( status === 'completed' ) goToNextStep();
}, [ completed ] );

@enejb
Copy link
Member

enejb commented Jun 19, 2024

I know this is not really part of this PR but it seems that the back button is currently broken. (Based on the video) Which seems like a bad user experience. Can we make the back button work again?

@jsnajdr
Copy link
Member Author

jsnajdr commented Jun 20, 2024

it seems that the back button is currently broken. [...] Which seems like a bad user experience.

The broken experience, is it that I can't really go back from /start/domains to /start/user, because the user step immediately redirects forward to domains again? This is happening since #65908 -- and in the discussion folks seem to welcome the new behavior: https://github.com/Automattic/wp-calypso/pull/65908/files#r934748021

What should be the correct experience? Going to the user step when clicking back, and staying there, and showing "continue as user" instead of the signup form? If we agree that this is the desired behavior, it's possible to implement, but not entirely easy. Because during the signup flow the user is already created, but not fully logged in. We have a bearer_token that we use to authenticate REST request, but that's all. The user is not set in Redux and the getCurrentUser selector will return null. We haven't yet gone through the WP login form and we don't have a full set of auth cookies. ContinueAsUser would have to be modified to accomodate all that.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Tested as expected for me, and the cleanup make sense - thanks for the thorough explanation and background @jsnajdr 🙌

I'm not sure how much the existing behavior should be a blocker for this cleanup PR, but I'll let @enejb decide.

@enejb
Copy link
Member

enejb commented Jul 5, 2024

Sorry for the slow response here. I don't think we should block this cleanup PR. So feel free to merge it.

I do think we should keep the browser back button working or offer them that acknowledges that they clicked the back button. There are a few reasons for this. What if they made a typo while spelling the email address. What if they are not ready to proceed and they would rather continue at a different time. But this is just my 2 cents. :)

@jsnajdr
Copy link
Member Author

jsnajdr commented Jul 8, 2024

Thanks for your reviews and comments! I think we can reconsider the back button behavior in near future, and change it to ContinueAsUser again. In a sense, this cleanup PR is actually enabling that, making the code easier to change.

@jsnajdr jsnajdr merged commit 9c95e74 into trunk Jul 8, 2024
19 of 20 checks passed
@jsnajdr jsnajdr deleted the update/signup-complete-state branch July 8, 2024 12:05
@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 Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants