-
Notifications
You must be signed in to change notification settings - Fork 893
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
Adds alert center redux store #21649
Conversation
Pull Request Test Coverage Report for Build d2aa0ec4ff7ec63ba5b6c5cb96cc410a3fa28b67Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
09358f2
to
c9090da
Compare
WIP on 271-add-problem-card-to-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 🏗️
const slice = createSlice( { | ||
name: "alertCenter", | ||
initialState: createInitialAlertCenterState(), | ||
extraReducers: ( builder ) => { |
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.
No toggle status or error support?
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 its something to ask UX what we want to do for error or request status?
03782f4
to
677ac37
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 ❓
Really liking the refactor to not pre-group 😄
I commented some more. Besides maybe the tests these are just that, comments. Feel free to leave them or address the tests in a follow-up?
|
||
export const alertCenterControls = { | ||
[ TOGGLE_ALERT_VISIBILITY ]: async( { payload } ) => { | ||
const formData = new URLSearchParams(); |
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.
If we ever run into encoding issues, we could try to switch this to FormData
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.
You could write tests for these?
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 added tests for the new components
I added tests for the store but it was difficult to test the happy path with the toggleAlert
with regular js object for a state.
daab8a5
to
a749b19
Compare
use global ajaxurl use ajaxurl from store
a749b19
to
0698366
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 && Acc: ✅
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:
Search engine visibility
in the Settings->Reading)wp-config.php
or snippets plugin:Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
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 Add Notifications to Alert center