-
Notifications
You must be signed in to change notification settings - Fork 23
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
Studio: Replace button loading indicator with full window one #362
Conversation
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.
I tested it and the happy path works great:
T0U7w7.mp4
I added a couple of suggestions and a small fix for an edge case where we selected the new site even if the user would have changed it.
Here is the video for the edge case:
1ssUzh.mp4
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.
The code change looks clear and works as expected. It works on Windows as well.
One note regarding UX - as creating a site on Windows takes more time than on MacOS, should we make the progress bar more dynamic to show some extrapolated progress?
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.
Thanks for the changes, I tested it again and it looks great.
7oEQ3D.mp4
I also tested the edge case where you create a site and then select another one. 👍
@@ -37,6 +38,8 @@ export default function AddSite( { className }: AddSiteProps ) { | |||
loadingSites, | |||
} = useAddSite(); | |||
|
|||
const isSiteAdding = data.some( ( site ) => site.isAddingSite ); |
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.
Nice!
It's better now, as the progress bar progresses for most of the process. We could improve it even more by adjusting the starting position to be closer to 20%, as now it almost immediately goes to ~50%, and we could make the progress slightly slower. Or maybe it should be enough to adjust starting position. |
Sounds good @wojtekn - I will add a separate PR to beautify this 👍 Added an issue here: https://github.com/Automattic/dotcom-forge/issues/8340 |
Related to https://github.com/Automattic/dotcom-forge/issues/8167
Proposed Changes
This PR replaces the loading state on the
Add site
modal with a full screen loading indicator:Testing Instructions
From "Add site" modal:
nvm use && npm install && npm start
Add site
button to open the modalAdd site
to add a siteAdd site
button is blocked for a given site when the site is being createdFrom "Onboarding" screen:
Add site
Testing for an error:
nvm use && npm install && npm start
Add site
button to open the modalAdd site
to add a siteNotes:
Pre-merge Checklist