-
Notifications
You must be signed in to change notification settings - Fork 52
Implement RTK Query approach for fetching wpcom sites #2132
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
Changes from all commits
c4efc6c
7d8fd66
ba89f0d
ee7ea22
afd3fbf
d7113b1
9a6f74f
7d0b2a5
b09206b
427d3ab
63cc6b8
48ffd67
7e1bbdb
c86048c
b134968
5610e0e
5079edf
99a6b55
5cfc0f2
8fa10dd
61e8a4b
a2fd40a
bc1776a
6f46ce6
cfaa8b4
ed59c46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ import { test, expect, type Page } from '@playwright/test'; | |
| import { E2ESession } from './e2e-helpers'; | ||
| import Onboarding from './page-objects/onboarding'; | ||
| import SiteContent from './page-objects/site-content'; | ||
| import WhatsNewModal from './page-objects/whats-new-modal'; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to fix a lint error, it not used |
||
| import { getUrlWithAutoLogin } from './utils'; | ||
|
|
||
| const global = globalThis as unknown as { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,11 +1,11 @@ | ||||||||||||||||||||||||
| import { cloudUpload } from '@wordpress/icons'; | ||||||||||||||||||||||||
| import { useI18n } from '@wordpress/react-i18n'; | ||||||||||||||||||||||||
| import Button from 'src/components/button'; | ||||||||||||||||||||||||
| import { Tooltip } from 'src/components/tooltip'; | ||||||||||||||||||||||||
| import { useCallback } from 'react'; | ||||||||||||||||||||||||
| import { useSyncSites } from 'src/hooks/sync-sites'; | ||||||||||||||||||||||||
| import { useAuth } from 'src/hooks/use-auth'; | ||||||||||||||||||||||||
| import { useContentTabs } from 'src/hooks/use-content-tabs'; | ||||||||||||||||||||||||
| import { useSiteDetails } from 'src/hooks/use-site-details'; | ||||||||||||||||||||||||
| import { ConnectButton } from 'src/modules/sync/components/connect-button'; | ||||||||||||||||||||||||
| import { useAppDispatch } from 'src/stores'; | ||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||
| connectedSitesActions, | ||||||||||||||||||||||||
|
|
@@ -24,30 +24,29 @@ export const PublishSiteButton = () => { | |||||||||||||||||||||||
| } ); | ||||||||||||||||||||||||
| const { isAnySitePulling, isAnySitePushing } = useSyncSites(); | ||||||||||||||||||||||||
| const isAnySiteSyncing = isAnySitePulling || isAnySitePushing; | ||||||||||||||||||||||||
| const handlePublishClick = () => { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const handlePublishClick = useCallback( () => { | ||||||||||||||||||||||||
| setSelectedTab( 'sync' ); | ||||||||||||||||||||||||
| dispatch( connectedSitesActions.openModal( 'push' ) ); | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| }, [ setSelectedTab, dispatch ] ); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if ( connectedSites.length !== 0 ) return null; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||
| <Tooltip | ||||||||||||||||||||||||
| disabled={ ! isAnySiteSyncing } | ||||||||||||||||||||||||
| text={ __( | ||||||||||||||||||||||||
| 'Another site is syncing. Please wait for the sync to finish before you publish your site.' | ||||||||||||||||||||||||
| ) } | ||||||||||||||||||||||||
| placement="left" | ||||||||||||||||||||||||
| <ConnectButton | ||||||||||||||||||||||||
| variant="primary" | ||||||||||||||||||||||||
| icon={ cloudUpload } | ||||||||||||||||||||||||
| connectSite={ handlePublishClick } | ||||||||||||||||||||||||
| disabled={ isAnySiteSyncing } | ||||||||||||||||||||||||
| tooltipText={ | ||||||||||||||||||||||||
| isAnySiteSyncing | ||||||||||||||||||||||||
| ? __( | ||||||||||||||||||||||||
| 'Another site is syncing. Please wait for the sync to finish before you publish your site.' | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| : __( 'Publishing your site requires an internet connection.' ) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+41
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is mostly a suggestion, but I think this would clarify how and when which tooltip text is displayed in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this suggestion but I would delay adding changes to this until #2171 is merged, at that point we can also refactor the |
||||||||||||||||||||||||
| > | ||||||||||||||||||||||||
| <Button | ||||||||||||||||||||||||
| variant="primary" | ||||||||||||||||||||||||
| disabled={ isAnySiteSyncing } | ||||||||||||||||||||||||
| aria-label={ __( 'Publish site' ) } | ||||||||||||||||||||||||
| onClick={ handlePublishClick } | ||||||||||||||||||||||||
| icon={ cloudUpload } | ||||||||||||||||||||||||
| > | ||||||||||||||||||||||||
| { __( 'Publish site' ) } | ||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||
| </Tooltip> | ||||||||||||||||||||||||
| { __( 'Publish site' ) } | ||||||||||||||||||||||||
| </ConnectButton> | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest moving more of this logic to connectSite: builder.mutation<
SyncSite[],
{ remoteSiteId: number; localSiteId: string; userId?: number }
>( {
queryFn: async ( { remoteSiteId, localSiteId, userId }, api ) => {
const connectedSites = await getIpcApi().getConnectedWpcomSites( localSiteId );
const { data: remoteSites = [] } = await api.dispatch(
wpcomSitesApi.endpoints.getWpComSites.initiate( {
connectedSiteIds: connectedSites.map( ( site ) => site.id ),
userId,
} )
);
const siteToConnect = remoteSites.find( ( site ) => site.id === remoteSiteId );
if ( ! siteToConnect ) {
return {
error: { status: 'CUSTOM_ERROR', error: 'Site not found in WordPress.com sites' },
};
}
await getIpcApi().connectWpcomSites( [
{
sites: [ siteToConnect ],
localSiteId,
},
] );
const actualConnectedSites = await getIpcApi().getConnectedWpcomSites( localSiteId );
return { data: actualConnectedSites };
},This would allow us to shorten this hook significantly: export function useListenDeepLinkConnection() {
const [ connectSite ] = useConnectSiteMutation();
const { selectedSite, setSelectedSiteId } = useSiteDetails();
const { setSelectedTab, selectedTab } = useContentTabs();
const { user } = useAuth();
useIpcListener( 'sync-connect-site', async ( _event, { remoteSiteId, studioSiteId } ) => {
if ( selectedSite?.id && selectedSite.id !== studioSiteId ) {
// Select studio site that started the sync
setSelectedSiteId( studioSiteId );
}
await connectSite( { remoteSiteId, localSiteId: studioSiteId, userId: user?.id } );
if ( selectedTab !== 'sync' ) {
// Switch to sync tab
setSelectedTab( 'sync' );
}
} );
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion, I will work on that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have been working on this, but I have found that this will trigger an additional call to the What do you think about tackling this specific refactor as a follow-up and progressing with the current changes?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds reasonable to tackle this in a follow-up. For context, my main goal here was to consolidate "business logic" in the Redux slices as much as possible, and make custom hooks and components passive consumers. In theory, the only reason this needs to be a hook rather than an event listener in the Redux slice is that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, I've created STU-1086 for that |
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.
This is to fix a lint error, it not used