Stepper: Automated going back behaviour with non-linear flow support ✨#101550
Stepper: Automated going back behaviour with non-linear flow support ✨#101550
Conversation
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~33 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~162 bytes removed 📉 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. 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. |
scruffian
left a comment
There was a problem hiding this comment.
This is working well for me. I left a few questions but nothing that should block this.
| /** | ||
| * If the `previousStep` is defined in the store, it's a solid proxy to guess that we navigated at least once via Stepper's React Router. | ||
| * If the flow doesn't define a `goBack` handler, and `previousStep` is defined, we can just go history.back() and we'll remain in the flow. | ||
| * But if `previousStep` is not defined, and the flow doesn't define a `goBack` handler, we should return undefined so the StepContainer doesn't render a back button. |
There was a problem hiding this comment.
Is it possible now for a flow to not define a goBack handler, since it will always inherit one?
There was a problem hiding this comment.
Yes exactly. If a flow defines one, it will supersede the default one. So not defining one will give your flow access to the default behavior. This may be shortsighted to say, but I think no flow should need to define its own goBack anymore.
There was a problem hiding this comment.
Is it worth offering a hybrid option so flows can only override the behaviour in specific situations? That would make it easier for flows to fall through to the default in most cases, and exceptions would not require the flow to implement handling for all possible cases.
There was a problem hiding this comment.
We can easily check the return value of the goBack from the flow and use that to decide flow.goBack?.() ?? defaultGoBack() but at this point, especially after your comment, I'm convinced we should remove going back from the flow level. It should always be aligned with the browser's back button.
There was a problem hiding this comment.
Actually, can this be a side-effect or secondary helper? In my mind, the only thing the flow should be doing is interacting with the flow state, largely in cases where the user might end up leaving the flow.
There was a problem hiding this comment.
I don't disagree, but I'm increasingly dispassionate about increasing the API surface area. Can we wait until we need that?
There was a problem hiding this comment.
I can think of an edge case that we don't have a good solution for: what happens when you go back to the processing step?
At present, users often get stuck because the original step that triggered an async action is the step before the processing step, and we haven't (re)triggered that original action.
cc @gabrielcaires as he has been thinking about this as a general Stepper issue/gap.
There was a problem hiding this comment.
Processing and site creation steps should replace history. Like this.
| }; | ||
| }, [] ); | ||
|
|
||
| const stepData = useSelect( |
There was a problem hiding this comment.
Is it better to return previous step from the store, rather than all the step data? Just thinking about the performance implications...
There was a problem hiding this comment.
I tried that, but the whole stepData object can be null, so I had to check it first before accessing previousStep.
| handleRecordStepNavigation( { | ||
| event: STEPPER_TRACKS_EVENT_STEP_NAV_GO_BACK, | ||
| } ); | ||
| history.back(); |
There was a problem hiding this comment.
I suppose another option could be to actually go to the previous step explicitly like navigate( previousStep ) - not real code! I'm not sure which is better...
There was a problem hiding this comment.
This was my initial approach, but it's bad because it pushes a new state down the browser's history stack when we want to pop. When I called navigate, going back via the browser went forward 🌀.
|
I'd love to take a deeper look but I may not be able to do it today - hope that' ok! In the meantime, I have a couple of high-level thoughts:
|
Yes, but this is an unescapable reality because the browser's go back button supersedes whatever Stepper principle we may come up with. We're better off aligning our design with it.
That would be an issue that I intentionally ignored. There are a few possibilities
I ignored this issue because when we move to
That's why I added a check to only activate it when you're not at the first step of the flow. Once you cross the first step, you'll have a valid |
|
If I think like a user for a moment, I always expect the Back button to work like the browser back button. I assume it will take me back to the last page I was on, whatever that was. So I would argue that our default behaviour should be something like:
Side note: I have strong feelings about any code that makes the Back button do anything that moves away from this approach! |
Can you explain what's a "deep" / "deeper" step? I may be missing some context 🙏 I also agree with @daledupreez 's general sentiment against hacking the history back behavior, and It's good to see that this PR is taking us closer to that vision. "Go back" until now really meant "go to the previous step according to the logic of the flow" , while we are proposing to change it to "go back to the previous browser history entry". Let's make sure that this change can be applied across the board without compromising all the different ways users experience the flows (including direct links to a step, page refreshes, chained flows, etc) |
I mean landing directly at a non-first step of a flow. If I'm being honest, my approach would be to make the back button simply call So this PR aims to call |
|
|
||
| /** | ||
| * If the previous step is defined in the store, and the current step is not the first step, we can go back. | ||
| * We need to make sure we're not at the first step because `previousStep` is persisted and can be a step from another flow or another run of the current flow. |
There was a problem hiding this comment.
Can previousStep (and any stale state) be cleanup automatically / expire when starting a new flow?
There was a problem hiding this comment.
Good idea. I wouldn't worry about that now though.
| const goBack = () => { | ||
| switch ( currentStepSlug ) { | ||
| case 'use-my-domain': | ||
| if ( getQueryArg( window.location.href, 'step' ) === 'transfer-or-connect' ) { | ||
| const destination = addQueryArgs( '/use-my-domain', { | ||
| ...getQueryArgs( window.location.href ), | ||
| step: 'domain-input', | ||
| initialQuery: getQueryArg( window.location.href, 'initialQuery' ), | ||
| } ); | ||
| return navigate( destination ); | ||
| } | ||
|
|
||
| if ( window.location.search ) { | ||
| window.history.replaceState( {}, document.title, window.location.pathname ); | ||
| } | ||
| return navigate( 'domains' ); | ||
| case 'plans': | ||
| if ( redirectedToUseMyDomain ) { | ||
| if ( Object.keys( useMyDomainQueryParams ).length ) { | ||
| // restore query params | ||
| const useMyDomainURL = addQueryArgs( 'use-my-domain', useMyDomainQueryParams ); | ||
| return navigate( useMyDomainURL ); | ||
| } | ||
| return navigate( 'use-my-domain' ); | ||
| } | ||
| return navigate( 'domains' ); | ||
| case 'domains': | ||
| if ( isGoalsAtFrontExperiment ) { | ||
| if ( isBigSkyBeforePlansExperiment && createWithBigSky ) { | ||
| return navigate( 'design-choices' ); | ||
| } | ||
| return navigate( 'design-setup' ); | ||
| } | ||
| case 'design-setup': | ||
| if ( isDesignChoicesStepEnabled ) { | ||
| return navigate( 'design-choices' ); | ||
| } | ||
| if ( isGoalsAtFrontExperiment ) { | ||
| return navigate( 'goals' ); | ||
| } | ||
| case 'difmStartingPoint': | ||
| return navigate( 'goals' ); | ||
| case 'design-choices': | ||
| return navigate( 'goals' ); | ||
| default: | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This is the kind of simplification we love to see! Thanks for seeing it through 🌟
Not sure how frequent this is, but I just came across such an example during my latest PR.
A few questions:
Another thought: if there isn't any history to navigate back to (case 3), should we just automatically hide the back button? And should we also consider hiding it if there isn't any "previous flow step" (case 2)? All I'm trying to say, is that we need to make sure this change won't cause a worse UX (and potentially, a loss of revenue). For example, on X, users are still able to click the "back" button and navigate back to a profile even when landing directly on a link (without previous history on the X website) — example. |
Yes, but the form should handle coming back to it. Stepper handles that, although poorly. If you reach checkout and go back, the site that was created before reaching checkout will be recycled.
It depends on where they came from I think. Really hard to tell.
Great idea. Implemented in
Yes, this PR does that already.
Yes, that was my goal when I implemented |
escapemanuele
left a comment
There was a problem hiding this comment.
It's so nice to see this happening 👏
|
Made a revert PR in case things go awry #101907 |
| * If the flow doesn't define a `goBack` handler, and `previousStep` is defined, we can just go history.back() and we'll remain in the flow. | ||
| * But if `previousStep` is not defined, and the flow doesn't define a `goBack` handler, we should return undefined so the StepContainer doesn't render a back button. | ||
| */ | ||
| ...( canUserGoBack && { |
There was a problem hiding this comment.
@alshakero Introducing this synthetic goBack function causes a brief flicker in the logged out onboarding flow when the user transitions from the signup step to the domains step.
There was a problem hiding this comment.
I think is a fix. Improves the canUserGoBack logic. #102022
|
The domains step randomly pops up for a second while transitioning from the CleanShot.2025-03-28.at.15.52.15.mp4Testing shows that reverting this PR fixes the problem 🙃 I this PR does make changes to the |
Fixes #101509
Proposed Changes
Because of my insistence to keep Stepper non-linear, I thought it was impossible to guess the correct going-back behavior on the framework level. And left that puzzle for each flow to piece together. But today I realized we can create a rock-solid default behavior for going back without the flows writing any code.
This is thanks to the
previousStepstore value that is updated on every Stepper navigation. The existence of this value implies that we're now at least 2 steps deep in the flow at hand, and simply callinghistory.back()whenpreviousStepis defined, guarantees that 1) we'll go to the previous step 2) we'll remain in the flow.This implements the most common use case for going back.
This also removes all the goBack logic from the onboarding flow because it's not needed anymore and to allow testing.
Why are these changes being made?
Defining
goBackfor every flow is too much work and needs maintenance.Testing Instructions