Fetch real API data in sync modal #608
Conversation
| <div className="a8c-subtitle-small text-black">{ __( 'Powered by' ) }</div> | ||
| <WordPressShortLogo className="h-4.5" /> |
src/hooks/use-sync-sites.ts
Outdated
| id: number; | ||
| name: string; | ||
| url: string; | ||
| isStaging: boolean; | ||
| stagingSiteIds: number[]; | ||
| syncSupport: SyncSupport; | ||
| }; | ||
|
|
||
| type Site = { |
There was a problem hiding this comment.
What do you think about adding a prefix?
Like ResponseSite or WpcomSite ?
There was a problem hiding this comment.
Sure. Even if this type isn't exported, I guess the name is short enough to be somewhat cryptic.
src/hooks/use-sync-sites.ts
Outdated
| if ( connectedSiteIds.some( ( id ) => id === site.ID ) ) { | ||
| return 'already-connected'; | ||
| } | ||
| if ( site.plan.product_id !== 1008 ) { |
There was a problem hiding this comment.
Let's check if the plan feature includes atomic or backups.
Something like: site.plan.features.active.includes('atomic')
If we prefer to use the plan number, then in addition to Business, we need to also support Pro and eCommerce, and also all the bundles like monthly, 2 years, 3 years.
Actually, all the plans that belong to the WPCOM_Features::ATOMIC array.
fbhepr%2Skers%2Sjcpbz%2Sjc%2Qpbagrag%2Szh%2Qcyhtvaf%2Sjcpbz%2Qsrngherf%2Spynff%2Qjcpbz%2Qsrngherf.cuc%3Se%3Q8p2742qn%23516%2Q521-og
There was a problem hiding this comment.
Good point about the Entrepreneur plan—that was an oversight on my part 👍 Regarding the Pro plan, I thought we shouldn't support those sites. From the PT (pesfPa-18P-p2):
[…] allow users to select production or staging sites with a Creator or higher hosting plan.
@wojtekn, could you confirm?
There was a problem hiding this comment.
|
This should be good for another look now, @sejas |
There was a problem hiding this comment.
@fredrikekelund , thanks for the changes!
I also think using the site features, and making it plan agnostic will be a better way to identify sites Sync compatible. What do you think?
src/hooks/use-sync-sites.ts
Outdated
| const BUSINESS_PLAN_ID = 1008; | ||
| const COMMERCE_PLAN_ID = 1011; |
There was a problem hiding this comment.
Let's make the Studio plan-agnostic if possible so it would only operate based on available features, e.g. atomic in this case.
Let's use site.plan.features.active.includes('atomic'). And if we want to limit the Pro plan, then we can add a condition for that specific plan.
If instead of using the site features, we prefer to use the plan ids, then we would need to add also all the bundles (monthly, 2-year, 3-year). Those have different plan IDs.
|
I've landed D164205-code and updated this PR accordingly. This should be good for a final (hopefully) look now 👍 |
Fixes https:/Automattic/dotcom-forge/issues/9465
Proposed Changes
Follow-up to #601. Implements an actual API call in
useSyncSites, along with some minor visual tweaks.Testing Instructions
STUDIO_SITE_SYNC=true npm startPre-merge Checklist