Stepper: Unify loading states #99158
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
escapemanuele
changed the title
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~1246 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~145288 bytes added 📈 [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 (~755 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. |
escapemanuele
force-pushed
the
stepper-loading-progress
branch
from
f3372f0 to
adf6316
Compare
matticbot
added
the
[Status] Needs Review
|
What would it take to move this component to a shared component. Should that be within |
|
@youknowriad, what you ask is right. And I didn't want to overcomplicate this. Maybe I could go on in the next PR, where I foresee unifying Checkout loading screen, and moving the loader to a shared component. |
| <div className="stepper-loader"> | ||
| <h1 className="stepper-loader__title processing-step__progress-step">{ title }</h1> | ||
| { renderProgressComponent() } | ||
| { subtitle && <p className="processing-step__subtitle">{ subtitle }</p> } |
There was a problem hiding this comment.
I've lost track a bit about how we add classes, but where are these coming from and why aren't they namespaced to stepper-loader?
There was a problem hiding this comment.
That's a great question. We should change this. These come from the processing step in Stepper but it's not guaranteed to be loaded in all flows.
There was a problem hiding this comment.
Totally right. It was a pre-existing class name taken from the Processing step. It is now stepper-loader__subtitle
| @@ -20,6 +18,7 @@ | |||
| margin: 0; | |||
| } | |||
|
|
|||
| .stepper-loader__progress-bar, | |||
There was a problem hiding this comment.
It would be better if we don't entangle the framework with the steps. Can we pass a class name to the progress bar from the processing step then stylize against that class?
There was a problem hiding this comment.
Nice catch. I removed it because it was not used anymore!
client/landing/stepper/declarative-flow/internals/components/stepper-loader/index.tsx
Outdated
Show resolved
Hide resolved
Now, I was thinking about something else, When validating a step, it seems there's always a small period of time when the next step is being loaded. We show the loader at that moment. I think it would be way more smooth if we find a way to preload the next step when the current step is visible. How possible would that be? I feel like these intermediary loaders are largely unnecessary and that there must be a way to avoid them entirely. |
|
I appreciate the time spent here @youknowriad.
Yes! That's the Calypso loading state. I have an issue to work on that after this and checkout work is done.
Yes, we I see a spinner there. I'll add this to the list of things, thank you for pointing that out!
Interesting. I'll put some thinking into this. |
client/landing/stepper/declarative-flow/internals/components/stepper-loader/index.tsx
Outdated
Show resolved
Hide resolved
|
A small thing I noticed (might be considered a follow-up similar to the other remarks) is that in addition to the progress bar the following step |
|
Testing this PR and trying to compare it with trunk. For the moment, I wouldn't say that this PR makes a significant difference on the user experience. It still feels that there's too much loaders and too much inconsistencies. That said, I think that's probably understandable because this one seems to focus on one specific loader only. I'm happy to move forward with this PR knowing that we'll tackle all the other inconsistencies in the follow-ups. |
| </div> | ||
| </div> | ||
| <> | ||
| <StepperLoader title={ __( 'Launching the AI Website Builder' ) } /> |
There was a problem hiding this comment.
The loading bar will stay rendered when there is an error.
| progress = -1, | ||
| className, | ||
| } ) => { | ||
| return ( | ||
| <div className={ clsx( 'stepper-loader', className ) }> | ||
| <h1 className="stepper-loader__title">{ title }</h1> | ||
| <ProgressBar | ||
| value={ progress >= 0 ? progress * 100 : undefined } |
There was a problem hiding this comment.
We should probably make the progress prop range from 0-100 and do the calculation on the consumer side. This way we can just do
| progress = -1, | |
| className, | |
| } ) => { | |
| return ( | |
| <div className={ clsx( 'stepper-loader', className ) }> | |
| <h1 className="stepper-loader__title">{ title }</h1> | |
| <ProgressBar | |
| value={ progress >= 0 ? progress * 100 : undefined } | |
| progress, | |
| className, | |
| } ) => { | |
| return ( | |
| <div className={ clsx( 'stepper-loader', className ) }> | |
| <h1 className="stepper-loader__title">{ title }</h1> | |
| <ProgressBar | |
| value={ progress } |
There was a problem hiding this comment.
I updated the codebase to use setProgress with 0-100 in mind. But to have the indefinite loading (the bar that doesn't follow numbers but goes on indefinitely), we have to not pass a number to value, as
const isIndeterminate = ! Number.isFinite( value );
Any suggestion?
There was a problem hiding this comment.
Now we're failing over to -1 when it's undefined, so if remove the -1, it will be undefined.
We do that, but the next step is a guess because Signup is not a series of consecutive steps and we sometimes Stepper guesses incorrectly. |
Can we pre-load all possibilities? Anyway, don't want to derail too much the conversation here but maybe there are options to explore separately here. |
That's what we did before. We preloaded all the steps once the site info was loaded. But turns out preloading too much is a net negative. Maybe we should preload the two most likely steps, that should cover 90% of the cases. |
This may be due to how the
I believe it makes some difference to slower devices, but to normal users with fast internet it will probably not change much. That said, I find this also is a good starting point for the unification and sharing of Loading components. With the next PRs it will become better and better. |
|
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 |
Yes, IIRC, the design step has some async logic of its own. |
|
Preloading improvements are ready for review #99352 |
|
Hi @Automattic/serenity! This PR touched on |
github-actions
bot
removed
the
[Status] Needs Review
Conversation: p1738260526318899/1738058408.252399-slack-C07H21B2W59
Fixes #99258
This PR will not change the W logo before the start of the flow and before checkout
Proposed Changes
Replace the pulsating WordPress logo with a ProgressBar during Onboarding, having the processing step and Stepper share the StepperLoader.
Why
The experience is really fragmented:
https://github.com/user-attachments/assets/6d53f2a7-e855-4714-8467-c7fcd2340635
Testing Instructions
/setup/onboardingFollowing work
Use
StepperLoader, or a more general step, also for Checkout here