-
Notifications
You must be signed in to change notification settings - Fork 291
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
Fix: Success banners remain present when the Audience Segmentation fe… #9681
base: develop
Are you sure you want to change the base?
Conversation
…ature is disabled.
Build files for 13ad7fa are ready:
|
Size Change: +262 B (+0.01%) Total Size: 1.88 MB
ℹ️ View Unchanged
|
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.
Nice work on this, thank you @ankitrox! I've left two very minor CR comments, please let me know if you have any questions.
await act( async () => { | ||
await registry | ||
.dispatch( CORE_USER ) | ||
.receiveGetDismissedItems( [ | ||
SETTINGS_VISITOR_GROUPS_SETUP_SUCCESS_NOTIFICATION, | ||
] ); | ||
} ); |
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'm having trouble understanding what this block actually does. Doesn't this make the test case technically wrong as the dismissal won't be trigger if it is already dismissed? I think in a general scenario this test would fail.
I've tried removing this and the test still passes, which I think is more appropriate.
await act( async () => { | |
await registry | |
.dispatch( CORE_USER ) | |
.receiveGetDismissedItems( [ | |
SETTINGS_VISITOR_GROUPS_SETUP_SUCCESS_NOTIFICATION, | |
] ); | |
} ); |
@@ -16,10 +16,15 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
/** | |||
* External dependencies |
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.
Nitpicky, but while we're at it:
* External dependencies | |
* WordPress dependencies |
…ature is disabled.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist