-
Notifications
You must be signed in to change notification settings - Fork 804
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
Revise Jetpack connection agreement text to comply with our User Agreement #28403
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
Backup plugin:
Boost plugin:
Social plugin:
Videopress plugin:
|
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run
to get started. More details: p9dueE-5Nn-p2 |
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 tested the PR via a JN site, and it works as advertised 👍 . I only have one comment.
projects/plugins/social/src/js/components/connection-screen/index.js
Outdated
Show resolved
Hide resolved
There also appears to be a problem with the social e2e testing and JS tests. Can you confirm it is not related to the changes? |
41bbcb3
to
b0f9dab
Compare
I noticed a few failing JS unit tests, so I spent time today to address those. Next up are the end-to-end tests for Social and Boost; I'll report back here with what I find. |
b0f9dab
to
3ec77b1
Compare
^ Replying to my earlier message: Boost tests were fine (i.e, I was mistaken); Social needed a small update to the connection test so that the correct 'Get Started'element was clicked. Previously only one element on the page had the text "Get Started"; but with our new terms of service notice, there's both a |
3ec77b1
to
749839d
Compare
…ect color more reliably
… connection screens
…t connection screen
…s of service on pricing table
…tch other buttons
fc1cd36
to
0ee901b
Compare
Looks like we also had a failing unit test related to some link text. I went ahead and fixed that as well. |
Thanks @mattgawarecki, I'll give it a final review first thing in the morning. |
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.
Looks great, thanks for working on this!
I also tested on WoA.
There isn't much to test really, as you can't disconnect there, but it doesn't seem to bring any issues either.
Fixes # 1202858161751496-as-1203651563192009/f
Proposed changes:
Other information:
Jetpack product discussion
p1HpG7-_kep-p2
Does this pull request change what data or activity we track or use?
Testing instructions:
jetpack build plugins/jetpack
wp-admin/admin.php?page=jetpack#/
)jetpack build plugins/backup
wp-admin/admin.php?page=jetpack-backup
)jetpack build plugins/boost
wp-admin/admin.php?page=jetpack-boost
)jetpack build plugins/social
wp-admin/admin.php?page=jetpack-social
)Build Jetpack plugin by running
jetpack build plugins/jetpack
Install and Navigate to the My Jetpack page (
wp-admin/admin.php?page=my-jetpack
) and ensure you still haven't connected your site.You will see the message as below and click on the button
On the connection screen confirm that Terms and condition text is displayed above the button