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

Close sync modal when creating new site #1142

Merged
merged 6 commits into from
Apr 7, 2025

Conversation

sejas
Copy link
Member

@sejas sejas commented Mar 27, 2025

Related issues

  • Fixes STU-254

Proposed Changes

  • Close Connect Sites Sync modal after connecting any site, including when the connection comes from a deep link.

Testing Instructions

TEST DEEP LINK

  • Run npm start
  • Select the Sync tab
  • Click on Connect Site
  • Click on "Create a new WordPress.com site"
  • Observe the browser opens
  • Observe the modal is still open, in case the user wants to go back to pick another site
  • Follow the process of purchase or access any atomic site home: https://wordpress.com/home/${WPCOM_SITE_SLUG}?studioSiteId=${YOUR_STUDIO_SITE_ID}
  • Click on the deep link
  • Observe Studio focus correctly, and the Sync modal closes
  • Observe the site is correctly connected
close-modal-after-deeplink.mp4

TEST REGULAR CONNECTION

  • Run npm start
  • Select the Sync tab
  • Click on Connect Site
  • Select a valid site
  • Click on connect
  • Observe the modal closes
  • Observe the site is correctly connected
connect-site-regular-flow.mp4

Pre-merge Checklist

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

@sejas sejas self-assigned this Mar 27, 2025
@sejas sejas requested a review from a team March 27, 2025 15:07
@sejas sejas changed the title Update/close sync modal when creating new site Close sync modal when creating new site Mar 27, 2025
@sejas sejas requested a review from wojtekn March 27, 2025 15:09
@wojtekn
Copy link
Contributor

wojtekn commented Mar 27, 2025

I've tested it using the HTML file:

<a href="wpcom-local-dev://sync-connect-site?remoteSiteId=206672177&studioSiteId=2a1d3553-1ee1-4fe5-9297-4aeff02d9248">Connect</a>
  1. Edit the file and fill out both site IDs
  2. Open the file in the browser
  3. Open Studio
  4. Make the other site than the one defined in the test file active, open the Sync tab and then the 'Connect site' window
  5. Click the Connect link in the test file

The PR doesn't fix this case. It seems that in this PR, we are changing the code to close the connection dialog immediately after the link is clicked. I'm unsure if it's the best approach, as the action the user just started is not yet finished, and keeping the dialog open would make more sense. User can change their mind, close the browser, and choose another site to connect in the connection dialog.

Should we update the sync-connect-site handle to close the dialog when the deep link is executed? And even further, to close any other dialog that could be open, e.g., 'Add site' or Settings?

@sejas
Copy link
Member Author

sejas commented Mar 27, 2025

@wojtekn, Thanks for testing the PR. This PR was made after exploring the other solution and it was an effort to find the simplest direct solution that solves user's pain.
I'll update this PR with the second approach, so the modal will still be open until the user finish the site connection.
I detailed both alternatives in STU-254 before implementing them, but in the end I thought to create the PR with the simple solution.

And even further, to close any other dialog that could be open, e.g., 'Add site' or Settings?

I see this a bit excessive at this moment. It would require a big refactor centralizing all the modals in the same Redux store. If you think it's necessary we can create a separate issue.

@wojtekn
Copy link
Contributor

wojtekn commented Mar 27, 2025

I see this a bit excessive at this moment. It would require a big refactor centralizing all the modals in the same Redux store. If you think it's necessary we can create a separate issue.

Agreed, I don't think we need that at this point.

@sejas
Copy link
Member Author

sejas commented Mar 28, 2025

This PR is ready for another review.

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.

Works as expected.

@sejas sejas merged commit 9eae33b into trunk Apr 7, 2025
9 checks passed
@sejas sejas deleted the update/close-sync-modal-when-creating-new-site branch April 7, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants