-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support media deep links #19768
Support media deep links #19768
Conversation
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
…gIntentReceiverActivity
↑ Updates: the logic of navigating to media screen in WPMainActivity
- Removes: the logic of launching media from ActivityLauncher.java + Adds: the logic of launching media in ActivityNavigator.kt ↑ Updates: the logic in DeepLinkNavigator to use ActivityNavigator.kt
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.
Hey @zwarm
I have tested and is working as expected. I made some minor changes. I am approving but not merging so that you can take a look at the changes I have made and revert if necessary.
override fun shouldHandleUrl(uri: UriWrapper): Boolean { | ||
return (uri.host == HOST_WORDPRESS_COM && | ||
uri.pathSegments.firstOrNull() == MEDIA_PATH) || uri.host == MEDIA_PATH | ||
} |
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 typo in the MEDIA_PATH variable, I fixed it with the commit b0b034c
android:host="wordpress.com" | ||
android:pathPattern="/media/..*" | ||
android:scheme="http" /> | ||
|
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.
That's weird ... we don't support .* for pages or stats or others.
initSelectedSite(); | ||
} | ||
mActivityNavigator.viewCurrentBlogMedia(this, getSelectedSite()); | ||
break; |
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.
Great idea - I forgot ActivityNavigator
existed.
@@ -114,5 +117,7 @@ class DeepLinkNavigator | |||
object OpenLoginPrologue : NavigateAction() | |||
data class OpenJetpackForDeepLink(val action: String?, val uri: UriWrapper) : NavigateAction() | |||
object OpenJetpackStaticPosterView : NavigateAction() | |||
data class OpenMediaForSite(val site: SiteModel) : NavigateAction() | |||
object OpenMedia : NavigateAction() |
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.
Updated the logic to use ActivityNavigator
here. Commit - 61fc769
This PR adds support for media deeplinks as a part of the Super fast media uploads
wordpress.com/media/{domain name}
intent://media/#Intent;scheme=jetpack;package=com.jetpack.android
To Test:
Preparing
test-media-links.html.zip
Testing the deep links
test-media-links.html
in a browser on your deviceIntent Media
linkwordpress.com/media
linkwordpress.com/media/nonsense
linkwordpress.com/media/..
for your domain linkRegression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
N/A
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.UI Changes Testing Checklist:N/A