-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
MetaBoxes: Saving meta boxes don't reset the comment/ping status #4865
Conversation
74bbf8d
to
4c72798
Compare
4c72798
to
645f67d
Compare
⌛️ I don't blame you 🙂 no one wants to review meta boxes. |
editor/store/effects.js
Outdated
@@ -498,11 +501,18 @@ export default { | |||
}, {} ); | |||
store.dispatch( setMetaBoxSavedData( dataPerLocation ) ); | |||
|
|||
// Additional data needed for backwards compatibility. | |||
// If we do not provide this data the post will be overriden with the default values. | |||
const additionData = [] |
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 can express it also as:
const additionalData = [
post[ 'comment_status' ] && `comment_status=${ post[ 'comment_status' ] }`,
post[ 'ping_status' ] && `ping_status=${ post[ 'ping_status' ] }`,
].filter( Boolean );
I learned this trick with filter recently :)
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.
Code looks good. I haven't tested yet.
editor/store/effects.js
Outdated
@@ -489,7 +492,7 @@ export default { | |||
}, {} ); | |||
store.dispatch( setMetaBoxSavedData( dataPerLocation ) ); | |||
}, | |||
REQUEST_META_BOX_UPDATES( action, store ) { | |||
REQUEST_META_BOX_UPDATES( { post }, store ) { |
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.
Does post
need to come through the action, or could we assume that the value in store.getState()
would be up to date by now?
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.
Yes, it's probably the same, I'll remove the post from the action
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.
Is it? I mean, it might be the same. It all depends how all those dispatch
actions are processed. This is quite tricky because you dispatch action in the middle of another dispatch. At the moment when you trigger requestMetaBoxUpdates
action, the payload of requestPostUpdateSuccess
is still processed, so there might be a different value in the store at that time. I think actions are dispatched asynchronously which means it should all end up as expected.
editor/store/effects.js
Outdated
@@ -498,11 +501,18 @@ export default { | |||
}, {} ); | |||
store.dispatch( setMetaBoxSavedData( dataPerLocation ) ); | |||
|
|||
// Additional data needed for backwards compatibility. | |||
// If we do not provide this data the post will be overriden with the default values. | |||
const additionData = [] |
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.
Typo: "addition" -> "additional"
editor/store/effects.js
Outdated
const formData = values( dataPerLocation ) | ||
.concat( jQuery( '.metabox-base-form' ).serialize() ) | ||
.concat( additionData ) | ||
.join( '&' ); |
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.
Aside: Would it be easier to construct this with FormData
instead?
Unfortunately not possible to pass an object to its constructor, needs to iterate with FormData#append
.†
But it can be passed directly into fetch
's body
property.
Above jQuery.serialize
would need to be revised to jQuery.serializeArray
.
I think FormData
manages encoding too, unlike the current code here where we concatenate post[ 'comment_status' ]
into the form without first encodeURIComponent
(not likely a real-world issue).
† There are helpers like object-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.
I’m afraid FormData doesn’t work properly with our codebase. We polyfill fetch with very limited support for FormData. At least I had a few issues with it a few weeks back. I don’t remember the details though 😃
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.
Please note the gaps for Edge and Safari: https://developer.mozilla.org/en-US/docs/Web/API/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.
I tried something similar the problem is that serializeArray
doesn't return a nested array when we have array parameters instead it returns something like { "fields[foo]": "bar" }
which is not the correct object we want.
editor/store/effects.js
Outdated
// Additional data needed for backwards compatibility. | ||
// If we do not provide this data the post will be overriden with the default values. | ||
const additionData = [] | ||
.concat( post[ 'comment_status' ] ? [ `comment_status=${ post[ 'comment_status' ] }` ] : [] ) |
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.
Unnecessary square brackets here on property access. Surprised this isn't a lint error.
I'm merging this one as it's a necessary fix for the release. |
closes #4843 and closes #4663
This PR fixes two related issues:
Testing instructions