-
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
SignupForm: remove legacy code for handling back button #91956
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~287 bytes removed 📉 [gzipped])
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])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
if ( | ||
prevProps.step?.status !== this.props.step?.status && | ||
this.props.step?.status === 'completed' | ||
) { |
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.
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 ] );
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? |
The broken experience, is it that I can't really go back from What should be the correct experience? Going to the |
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.
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. :) |
Thanks for your reviews and comments! I think we can reconsider the back button behavior in near future, and change it to |
The
user
step in signup has one problem that it needs to solve: if theuser
step is completed and the user account is created, you proceed to the next step, likedomains
. Now what happens when you click the back button? You go back touser
. 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 theuser
step iscompleted
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 theuser
step iscompleted
. 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 showsContinueAsUser
is when it's the only step in the flow, like inaccount
. But there's no next step to go back from. After theuser
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, thedetectHistoryNavigation
usages. One of the consequences is that many of theuser
components no longer need thestep
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:
step.status === 'completed'
state where theUser
step renders nothing./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 singleuser
step.