-
Notifications
You must be signed in to change notification settings - Fork 51
Implement new site launch and import buttons #2002
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
Conversation
📊 Performance Test ResultsComparing 8e46a5a vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
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.
Pull Request Overview
This PR refactors the sync functionality to provide separate "Publish" and "Pull" actions instead of a generic "Connect site" button. The changes introduce a modal mode system that allows users to explicitly choose whether they want to push content to a remote site or pull content from one.
Key Changes:
- Replaced single "Connect site" button with separate "Publish site" and "Pull site" buttons
- Added
SyncModalModetype to track the operation mode ('push'|'pull'|'connect') - Updated modal titles and button labels to reflect the selected operation mode
- Integrated
SyncDialogcomponent to handle push/pull operations after site selection
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/sync/types.ts | Added SyncModalMode type definition |
| src/modules/sync/index.tsx | Refactored to support separate push/pull workflows with modal state management |
| src/modules/sync/tests/index.test.tsx | Updated test cases to verify publish and import button functionality |
| src/modules/sync/components/sync-sites-modal-selector.tsx | Added mode-aware modal titles, button labels, and offline messages |
| src/modules/sync/components/connect-button.tsx | Made tooltip text configurable via props |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sejas
left a comment
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.
Cool! I confirm that clicking on Publish site let me choose the site and then opens the Selective sync modal. I think works as a first iteration, but I would prefer if the modal is the "same" and it doesn't blink.
Also, I wonder if the connection should only happen after the user actually clicks on the final Push or Pull. If the user selects a remote site and then cancels the second modal, I would expect that no sites have been connected yet. That's my personal opinion, so I'm open to other opinions.
As I said, I think this is a good first iteration and I think we can merge it after 1.6.3 is out, and create a follow-up to polish the UX.
I also left some suggestions, like renaming the first modal button to Next and maybe moving the modalState to the Redux slice.
try-pull-push-main-buttons.mp4
src/modules/sync/index.tsx
Outdated
| setModalState( ( prev ) => ( { ...prev, mode: 'push' } ) ); | ||
| dispatch( connectedSitesActions.openModal() ); |
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 wonder if the modalState could be part of the Redux slice and the openModal could receive the mode as parameter.
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.
That is a great suggestion! I have actually needed to do it on #2025, I can do it in this PR though so we test with the final code
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've gone ahead and moved to Redux, it simplifies the code a lot 38d8ae4
src/modules/sync/index.tsx
Outdated
| dispatch( connectedSitesActions.closeModal() ); | ||
| setModalState( ( prev ) => ( { ...prev, mode: null } ) ); |
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.
If the modalState would live inside the Redux slice, then we could clear the mode inside closeModal.
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.
That's absolutely right, done as part of 38d8ae4
|
Thanks @sejas for your review!
That's a good point, I was hesitant to change that or not after initial implementation, but I agree it can be surprising to users if the site is connected even if they click cancel. I have implemented the change 953be86. Now, there is another blink when pulling/pushing and listing the site, but that can also be tackled as a follow-up. If we decide to go with the Connect always approach, the commit can be reverted easily. Additionally, I have implemented all your suggestions, so I would appreciate another quick look if possible 🙏 |
sejas
left a comment
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.
@epeicher , great work. Thanks addressing the feedback. Connecting the site in the last step is a huge improvement. I'll create a low priority issue to fix the blink between modals. Let's merge the PR once the V1.6.3 is out and the channel is green.
https://github.com/user-attachments/assets/5a7a1548-bfa4-4d6e-8daa-c18a44108a5b
| tooltipText={ __( 'Importing a remote site requires an internet connection.' ) } | ||
| > | ||
| { __( 'Connect site' ) } | ||
| { __( 'Pull site' ) } |
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.
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.
Well spotted! Changed as part of 51f1e67 in a similar way than the Export database button. Ideally, we should not need to override the style buttons, but I would tackle that as a follow-up, as it implies checking all the Buttons used to avoid regressions
| __( 'Push and pull changes from your live site.' ), | ||
| __( 'Connect multiple environments.' ), | ||
| __( 'Supports staging and production sites.' ), | ||
| __( 'Sync database and file changes.' ), |
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 am finding file changes in there to sounding a bit strange, I would change it to something simpler like:
| __( 'Sync database and file changes.' ), | |
| __( 'Sync files and database.' ), |
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.
katinthehatsite
left a comment
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 work, everything seems to be working as expected for me ❤️
I found an edge case where I was able to start push process for two different sites at the same time (it seems that it is not intentional). This is how I did it:
- On a Studio site A, that does not have any sites connected, navigate to
Synctab - Start either
PullorPushpush from the modal - Switch to a Studio site B that does not have any connections
- Start another push or pull processes from the modal
- Observe two push/ pull processes running at the same time
- If Studio site B has sites connected already, then the
PushandPullbuttons are correctly disabled
katinthehatsite
left a comment
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.
Also, this is maybe a suggestion, I was thinking that it would be cool to have a Back button in the connection screen so that you can navigate back from the Sync modal to connection modal if you change your mind about the site you want to connect.
This definitely does not need to be implemented in this PR, we can just add this for consideration for @Marinatsu
|
Thanks @katinthehatsite for your review!
Well spotted! I think we should apply the same logic than to Pull and Push buttons, and disabled them when there is another site syncing, what do you think @sejas ? This would be the result:
|
This makes sense to me 👍 |
|
Great addition! Thanks for making it consistent ✨ |
| interface ConnectButtonProps { | ||
| variant: ButtonVariant; | ||
| connectSite?: () => void; | ||
| disableConnectButtonStyle?: boolean; |
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 was always used as true, so it has been removed, and the functionality updated



Related issues
CleanShot.2025-11-05.at.18.07.23.mp4
Design RToz6tIuQ7nlZrikBte4GU-fi-10114_133424
Proposed Changes
Publish siteandPull sitebuttons to replace the singleConnect sitebuttonTesting Instructions
Start Studio and select a local site
Navigate to the Sync tab
Log in to WordPress.com if not already authenticated
Test Publish Flow:
Publish sitebuttonPublish your sitePush to [Environment]Pushand check there are no errorsTest Pull Flow:
Pull sitebuttonSelect a site to importPull from [Environment]Pulland check the pull is successTest Connect Flow (when sites already exist):
Connect another sitebuttonConnect your siteTest Offline Mode:
Publish siteandPull siteshows tooltip with a meaningful messagePre-merge Checklist