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

Studio: Replace button loading indicator with full window one #362

Merged
merged 19 commits into from
Jul 17, 2024

Conversation

katinthehatsite
Copy link
Contributor

@katinthehatsite katinthehatsite commented Jul 15, 2024

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:

Screenshot 2024-07-16 at 9 02 33 AM

Testing Instructions

From "Add site" modal:

  • Pull the changes from this branch
  • Start the app with nvm use && npm install && npm start
  • Click on the Add site button to open the modal
  • Click on Add site to add a site
  • Confirm that you see the loading state that takes the whole screen
  • Confirm that you see the loading indicator in the sidebar
  • Confirm that when you switch to a different site, you see the content tabs as expected
  • Confirm that the Add site button is blocked for a given site when the site is being created

From "Onboarding" screen:

  • Pull the changes from this branch
  • Start the app with `nvm use && npm install && npm start
  • Delete all the sites that you have to trigger the onboarding screen
  • Click on Add site
  • Confirm that you see the loading state that takes the whole screen
  • Confirm that you see the loading indicator in the sidebar

Testing for an error:

  • Apply the following diff:
diff --git a/src/ipc-handlers.ts b/src/ipc-handlers.ts
index 95903bb..b2c85d3 100644
--- a/src/ipc-handlers.ts
+++ b/src/ipc-handlers.ts
@@ -130,6 +130,9 @@ export async function createSite(
 ): Promise< SiteDetails[] > {
        const userData = await loadUserData();
        const forceSetupSqlite = false;
+
+       // Introduce an error for testing
+       throw new Error( 'Test Error: This is a forced error for testing purposes.' );
        // We only recursively create the directory if the user has not selected a
        // path from the dialog (and thus they use the "default" or suggested path).
        if ( ! ( await pathExists( path ) ) && path.startsWith( DEFAULT_SITE_PATH ) ) {
  • Start the app with nvm use && npm install && npm start
  • Click on the Add site button to open the modal
  • Click on Add site to add a site
  • Observe that you see the loading state
  • Observe that it throws the dialog with an error
  • Observe that the loading state goes away and the site is not created

Notes:

  • I am not sure what the error handling is supposed to be like according to the design at the moment. For now, I implemented something that seems to make sense and does not look too strange for the user experience. We can open another PR once we have some more clarify on what the design and flow for the error + loading state should look like
  • I am planning to more test coverage in a different PR to make this code available for the PR about import interface as soon as possible

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@katinthehatsite katinthehatsite self-assigned this Jul 15, 2024
@katinthehatsite katinthehatsite changed the title https://github.com/Automattic/dotcom-forge/issues/8167 Studio: Replace button loading indicator with full window one Jul 15, 2024
Copy link
Member

@sejas sejas left a 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

src/components/add-site.tsx Outdated Show resolved Hide resolved
src/components/add-site.tsx Show resolved Hide resolved
src/hooks/use-add-site.ts Show resolved Hide resolved
src/hooks/use-site-details.tsx Outdated Show resolved Hide resolved
@katinthehatsite katinthehatsite requested review from sejas and a team July 17, 2024 09:19
Copy link
Contributor

@wojtekn wojtekn left a 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?

src/components/add-site.tsx Show resolved Hide resolved
src/components/site-loading-indicator.tsx Show resolved Hide resolved
@katinthehatsite
Copy link
Contributor Author

@wojtekn would you be able to test on Windows again (I made some changes in d83fd0d ). It looks better on Mac and I would like to confirm that there is an improvement on Windows. Thank you!

Copy link
Member

@sejas sejas left a 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 );
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@katinthehatsite katinthehatsite merged commit 49312d6 into trunk Jul 17, 2024
10 checks passed
@katinthehatsite katinthehatsite deleted the add/ui-for-loading-indicator branch July 17, 2024 17:05
@wojtekn
Copy link
Contributor

wojtekn commented Jul 18, 2024

@wojtekn would you be able to test on Windows again (I made some changes in d83fd0d ). It looks better on Mac and I would like to confirm that there is an improvement on Windows. Thank you!

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.

@katinthehatsite
Copy link
Contributor Author

katinthehatsite commented Jul 18, 2024

Sounds good @wojtekn - I will add a separate PR to beautify this 👍 Added an issue here: https://github.com/Automattic/dotcom-forge/issues/8340

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.

3 participants