From e21e79c290b21255135ddab01de1f4c85564ade8 Mon Sep 17 00:00:00 2001 From: Ivan Ottinger <25105483+ivan-ottinger@users.noreply.github.com> Date: Tue, 19 Nov 2024 17:34:35 +0100 Subject: [PATCH 1/7] Show spinner when the site is being pulled --- src/components/site-menu.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/components/site-menu.tsx b/src/components/site-menu.tsx index 51432d4b17..0db109968f 100644 --- a/src/components/site-menu.tsx +++ b/src/components/site-menu.tsx @@ -4,6 +4,7 @@ import { __, sprintf } from '@wordpress/i18n'; import { useEffect } from 'react'; import { useImportExport } from '../hooks/use-import-export'; import { useSiteDetails } from '../hooks/use-site-details'; +import { useSyncSites } from '../hooks/sync-sites'; import { isMac } from '../lib/app-globals'; import { cx } from '../lib/cx'; import Tooltip from './tooltip'; @@ -108,9 +109,13 @@ function ButtonToRun( { running, id, name }: Pick< SiteDetails, 'running' | 'id' } function SiteItem( { site }: { site: SiteDetails } ) { const { selectedSite, setSelectedSiteId } = useSiteDetails(); - const { isSiteImporting } = useImportExport(); const isSelected = site === selectedSite; + const { isSiteImporting } = useImportExport(); + const { isSiteIdPulling } = useSyncSites(); const isImporting = isSiteImporting( site.id ); + const isPulling = isSiteIdPulling( site.id ); + const showSpinner = site.isAddingSite || isImporting || isPulling; + return (
  • { site.name } - { site.isAddingSite || isImporting ? ( + { showSpinner ? ( ) : ( From 920ee77b1eb346b3ca3847357781a5fd12397196 Mon Sep 17 00:00:00 2001 From: Ivan Ottinger <25105483+ivan-ottinger@users.noreply.github.com> Date: Tue, 19 Nov 2024 17:44:39 +0100 Subject: [PATCH 2/7] Fix linting error --- src/components/site-menu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/site-menu.tsx b/src/components/site-menu.tsx index 0db109968f..0f3845c4d9 100644 --- a/src/components/site-menu.tsx +++ b/src/components/site-menu.tsx @@ -2,9 +2,9 @@ import { speak } from '@wordpress/a11y'; import { Spinner } from '@wordpress/components'; import { __, sprintf } from '@wordpress/i18n'; import { useEffect } from 'react'; +import { useSyncSites } from '../hooks/sync-sites'; import { useImportExport } from '../hooks/use-import-export'; import { useSiteDetails } from '../hooks/use-site-details'; -import { useSyncSites } from '../hooks/sync-sites'; import { isMac } from '../lib/app-globals'; import { cx } from '../lib/cx'; import Tooltip from './tooltip'; From 123890cb6fb40996971e52d8383c6b99251e8517 Mon Sep 17 00:00:00 2001 From: Ivan Ottinger <25105483+ivan-ottinger@users.noreply.github.com> Date: Wed, 20 Nov 2024 10:20:59 +0100 Subject: [PATCH 3/7] Unit tests: Ensure `MainSidebar` is wrapped with `SyncSitesProvider` --- src/components/tests/main-sidebar.test.tsx | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/components/tests/main-sidebar.test.tsx b/src/components/tests/main-sidebar.test.tsx index c235fc5820..790bfd9c88 100644 --- a/src/components/tests/main-sidebar.test.tsx +++ b/src/components/tests/main-sidebar.test.tsx @@ -1,4 +1,5 @@ import { render, act, screen } from '@testing-library/react'; +import { SyncSitesProvider } from '../../hooks/sync-sites'; import { userEvent } from '@testing-library/user-event'; import { useAuth } from '../../hooks/use-auth'; import MainSidebar from '../main-sidebar'; @@ -55,46 +56,50 @@ jest.mock( '../../hooks/use-site-details', () => ( { useSiteDetails: () => ( { ...siteDetailsMocked } ), } ) ); +const renderWithProvider = ( children: React.ReactElement ) => { + return render( { children } ); +}; + describe( 'MainSidebar Footer', () => { beforeEach( () => { jest.clearAllMocks(); } ); it( 'Has add site button', async () => { ( useAuth as jest.Mock ).mockReturnValue( { isAuthenticated: false } ); - await act( async () => render( ) ); + await act( async () => renderWithProvider( ) ); expect( screen.getByRole( 'button', { name: 'Add site' } ) ).toBeVisible(); } ); it( 'applies className prop', async () => { const { container } = await act( async () => - render( ) + renderWithProvider( ) ); expect( container.firstChild ).toHaveClass( 'test-class' ); } ); it( 'shows a "Stop All" button when there are running sites', async () => { - await act( async () => render( ) ); + await act( async () => renderWithProvider( ) ); expect( screen.getByRole( 'button', { name: 'Stop all' } ) ).toBeVisible(); } ); } ); describe( 'MainSidebar Site Menu', () => { it( 'renders the list of sites', async () => { - await act( async () => render( ) ); + await act( async () => renderWithProvider( ) ); expect( screen.getByRole( 'button', { name: 'test-1' } ) ).toBeVisible(); expect( screen.getByRole( 'button', { name: 'test-2' } ) ).toBeVisible(); expect( screen.getByRole( 'button', { name: 'test-3' } ) ).toBeVisible(); } ); it( 'has "start site" buttons when sites are not running', async () => { - await act( async () => render( ) ); + await act( async () => renderWithProvider( ) ); expect( screen.getByRole( 'button', { name: 'start test-1 site' } ) ).toBeVisible(); expect( screen.getByRole( 'button', { name: 'start test-2 site' } ) ).toBeVisible(); } ); it( 'starts a site', async () => { const user = userEvent.setup(); - await act( async () => render( ) ); + await act( async () => renderWithProvider( ) ); const greenDotFirstSite = screen.getByRole( 'button', { name: 'start test-1 site' } ); expect( greenDotFirstSite ).toBeVisible(); await user.click( greenDotFirstSite ); @@ -105,7 +110,7 @@ describe( 'MainSidebar Site Menu', () => { it( 'stops a site', async () => { const user = userEvent.setup(); - await act( async () => render( ) ); + await act( async () => renderWithProvider( ) ); const greenDotFirstSite = screen.getByRole( 'button', { name: 'stop test-3 site' } ); expect( greenDotFirstSite ).toBeVisible(); await user.click( greenDotFirstSite ); From 5f328958bf7cb84d4bd25bf4f1dd050af4e6a08f Mon Sep 17 00:00:00 2001 From: Ivan Ottinger <25105483+ivan-ottinger@users.noreply.github.com> Date: Wed, 20 Nov 2024 10:29:32 +0100 Subject: [PATCH 4/7] Fix linting error --- src/components/tests/main-sidebar.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/tests/main-sidebar.test.tsx b/src/components/tests/main-sidebar.test.tsx index 790bfd9c88..76cc662a43 100644 --- a/src/components/tests/main-sidebar.test.tsx +++ b/src/components/tests/main-sidebar.test.tsx @@ -1,6 +1,6 @@ import { render, act, screen } from '@testing-library/react'; -import { SyncSitesProvider } from '../../hooks/sync-sites'; import { userEvent } from '@testing-library/user-event'; +import { SyncSitesProvider } from '../../hooks/sync-sites'; import { useAuth } from '../../hooks/use-auth'; import MainSidebar from '../main-sidebar'; From 46851f2731d653cdf20776953abda1df6ffe299a Mon Sep 17 00:00:00 2001 From: Ivan Ottinger <25105483+ivan-ottinger@users.noreply.github.com> Date: Wed, 20 Nov 2024 16:56:24 +0100 Subject: [PATCH 5/7] Add tooltip text --- src/components/site-menu.tsx | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/components/site-menu.tsx b/src/components/site-menu.tsx index 0f3845c4d9..b4f69d34dc 100644 --- a/src/components/site-menu.tsx +++ b/src/components/site-menu.tsx @@ -116,6 +116,14 @@ function SiteItem( { site }: { site: SiteDetails } ) { const isPulling = isSiteIdPulling( site.id ); const showSpinner = site.isAddingSite || isImporting || isPulling; + const tooltipText = site.isAddingSite + ? __( 'Adding' ) + : isImporting + ? __( 'Importing' ) + : isPulling + ? __( 'Pulling' ) + : __( 'Loading' ); + return (
  • { showSpinner ? ( - + +
    + +
    +     ) : ( ) } From 676b371c1969e321fcf480e6965684687a1503b8 Mon Sep 17 00:00:00 2001 From: Ivan Ottinger <25105483+ivan-ottinger@users.noreply.github.com> Date: Thu, 21 Nov 2024 10:18:23 +0100 Subject: [PATCH 6/7] Replace `Pulling` with `Syncing` --- src/components/site-menu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/site-menu.tsx b/src/components/site-menu.tsx index b4f69d34dc..8eb49b94f0 100644 --- a/src/components/site-menu.tsx +++ b/src/components/site-menu.tsx @@ -121,7 +121,7 @@ function SiteItem( { site }: { site: SiteDetails } ) { : isImporting ? __( 'Importing' ) : isPulling - ? __( 'Pulling' ) + ? __( 'Syncing' ) : __( 'Loading' ); return ( From a35255914338a3a67fb17022c32f6031c82d4df1 Mon Sep 17 00:00:00 2001 From: Ivan Ottinger <25105483+ivan-ottinger@users.noreply.github.com> Date: Thu, 21 Nov 2024 10:55:10 +0100 Subject: [PATCH 7/7] Replace ternary with if-else for better readability --- src/components/site-menu.tsx | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/components/site-menu.tsx b/src/components/site-menu.tsx index 8eb49b94f0..32b94c37e8 100644 --- a/src/components/site-menu.tsx +++ b/src/components/site-menu.tsx @@ -116,13 +116,16 @@ function SiteItem( { site }: { site: SiteDetails } ) { const isPulling = isSiteIdPulling( site.id ); const showSpinner = site.isAddingSite || isImporting || isPulling; - const tooltipText = site.isAddingSite - ? __( 'Adding' ) - : isImporting - ? __( 'Importing' ) - : isPulling - ? __( 'Syncing' ) - : __( 'Loading' ); + let tooltipText; + if ( site.isAddingSite ) { + tooltipText = __( 'Adding' ); + } else if ( isImporting ) { + tooltipText = __( 'Importing' ); + } else if ( isPulling ) { + tooltipText = __( 'Syncing' ); + } else { + tooltipText = __( 'Loading' ); + } return (