-
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
[Jetpack Plugin Install] Reuse the tweaked UI for remote Jetpack install #18058
Conversation
📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr18058-d8c78c8.apk
|
📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr18058-d8c78c8.apk
|
267d304
to
6595050
Compare
d8c78c8
to
96de52a
Compare
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
Generated by 🚫 dangerJS |
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
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 changes, @thomashorta
It works as expected. I've left a question, but LGTM
} | ||
|
||
override fun onBackPressed() { | ||
if (viewModel.liveViewState.value?.showCloseButton == false) return |
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.
💡 Would it make sense to move this if
and the trackWithSource
call to the ViewModel?
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 think it will not make a lot of difference in the code here as I still need to return some information saying the back should be blocked, but it's better in terms of testing the analytics. I will do the change.
… into issue/18045-jp-install-reuse-ui
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.
LGTM
Fixes #18045
Update the existing "Remote Install" Flow to use this same updated flow (made with Compose). This flow is triggered when the user clicks a Jetpack feature (such as Stats) without having the Jetpack plugin installed on their self-hosted site.
Demo
Success Flow
issue-18045-jp-install-remote-success-blur.mov
Error Flow
issue-18045-jp-install-remote-error.mp4
To test
Set up a self-hosted site that has only no Jetpack plugins at all.
Regression Notes
Potential unintended areas of impact
Changes in installation behavior.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual test (it is working exactly as it was before).
What automated tests I added (or what prevented me from doing so)
Updated existing tests.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.