-
Notifications
You must be signed in to change notification settings - Fork 894
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
Show notification when alert fails to change visibility #21671
Show notification when alert fails to change visibility #21671
Conversation
Pull Request Test Coverage Report for Build 454222fbe114ec259c0357270e4253d261a44a2cDetails
💛 - Coveralls |
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.
CR 🏗️
What should happen when you click multiple and they all go wrong?
We should probably also just toggle the notification, regardless of the error. At least this session the intention was clear
4428eea
to
e9dbd46
Compare
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.
CR 🏗️
AT ❓
Works nicely in general!
As you noted yourself, the copy of hidden/shown might mismatch
When collapsing the WP menu, the notification does not match it like we do in settings. Does not seem that important to be fair!
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.
This seems overkill in a helper? Plus the naming implicates it can be used for anything, while in reality it is very much tied to the alert center?
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.
CR ✅
Besides a new ESLint warning and a conflict with trunk
😬
2a332eb
to
dae2b09
Compare
AC ✅ |
Tested "something went wrong" notification by blocking admin-ajax.php request and by cutting of network. |
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Yoast SEO
->General
Network
tab of your browser console, either block the requests containingadmin-ajax.php
or simulate being offlinex
buttonThis *notification* can't be hidden at this time. Please try again later.
, andThis problem can't be hidden at this time. Please try again later.
otherwiseRelevant test scenarios
Test instructions for QA when the code is in the RC
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #282