-
Notifications
You must be signed in to change notification settings - Fork 2k
Sync: Update Studio URL scheme to open sync after hosted site flow #107172
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
Sync: Update Studio URL scheme to open sync after hosted site flow #107172
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
c3b9d61 to
1009a0e
Compare
ivan-ottinger
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. The changes look good and work as expected. 👌🏼
Tested with both Studio trunk and the Automattic/studio#2029 branch. In both cases, the new WPCOM site got connected correctly.
I see we are not handling a case where the user does not have the Studio app installed, but that is very edge case - since user gets into this flow with the app already opened. Plus, we haven't addressed this edge case before either.
Thanks for the clear step-by-step testing instructions.
I left one comment below, but I think we are good merging the PR as-is.
|
|
||
| // Fallback to legacy scheme for old Studio app versions. It will be blocked by the browser | ||
| // if scheme is not registered. | ||
| setTimeout( () => { |
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.
Looking at other possible solutions, we could also consider listening to the blur event that should trigger when the browser tries to open the link. If this event isn't triggered, that could be a signal for the fallback URL to be used.
However, I believe that the current solution should be sufficient.
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.
Thanks for the suggestion, @ivan-ottinger. I had also tried that approach, if I recall correctly. It was not reliably opening the Studio with the fallback link.
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.
Thanks! Makes sense. 👍🏼
34445e6 to
7bbe6fd
Compare
Yes. That is intentional. As far as I understand, these links are only used in the flow that originates from Studio. So we can safely assume the user has Studio installed. |
Part of STU-936
Proposed Changes
wpcom-local-dev://towp-studio://while maintaining backward compatibility for older versions.Why are these changes being made?
Testing Instructions
Apologies for the long test instructions.
trunkbranch./home/250259110?studioSiteId=d67f9e58-97da-41d9-b99d-561d9f0b841dIt must contain the
studioSiteIdquery parameter.wpcom-local-devtowp-studiostudio#2029.studioSiteIdparameter.Pre-merge Checklist