-
Notifications
You must be signed in to change notification settings - Fork 823
Connection: adapt js component for custom errors #44281
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
base: trunk
Are you sure you want to change the base?
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 7 files. Only the first 5 are listed here.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
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.
Works well, as far as I can tell. Just a couple of suggestions, otherwise good to go 👍
projects/js-packages/connection/components/connection-error-notice/index.jsx
Outdated
Show resolved
Hide resolved
projects/js-packages/connection/hooks/use-connection-error-notice/index.jsx
Outdated
Show resolved
Hide resolved
Thank you for reviewing this and for your suggestions @sergeymitr ! I made changes to address both recommendations. |
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.
Works great!
Previously in #43593 we added support for Protected owner errors, but due to how the Error Handler was built, we hardcoded the error type into PHP and js files.
This PR is continuing the work started in #44042 where we refactored the PHP side of connection error handling so nothing has to be hardcoded. Now we are removing the protected owner errors from js files and adding support for dynamically added custom errors.
Errors are added to PHP via filter (currently limited to WoA sites), and they contain data used by JS to build error notification and actions ( message text, action type, tracking code). The Error Handler class now includes a helper function to construct a valid error_data array.
Additionally, PHP and JS can now handle custom errors with two action buttons. The secondary button is optional, but we already know we will need it for WoA pghT6q-6h-p2#comment-34.
Default connection errors should work as usual.
Closes VULCAN-184
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
wpcomsh
.temp_add_protected_owner_error()
to display one or two buttons and edit other parameters.