-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Save meta on preview #11409
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
Save meta on preview #11409
Changes from all commits
0c46e21
b17b85f
8e54535
acde4a5
20691f3
89660bc
ecf175d
ee275cc
a842446
b757300
74035ca
8ab9863
f2f1514
da53820
de2a5f1
7e707c7
99f5bca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,24 +45,26 @@ const effects = { | |
|
|
||
| let wasSavingPost = select( 'core/editor' ).isSavingPost(); | ||
| let wasAutosavingPost = select( 'core/editor' ).isAutosavingPost(); | ||
| let wasPreviewingPost = select( 'core/editor' ).isPreviewingPost(); | ||
| // Save metaboxes when performing a full save on the post. | ||
| subscribe( () => { | ||
| const isSavingPost = select( 'core/editor' ).isSavingPost(); | ||
| const isAutosavingPost = select( 'core/editor' ).isAutosavingPost(); | ||
| const isPreviewingPost = select( 'core/editor' ).isPreviewingPost(); | ||
| const hasActiveMetaBoxes = select( 'core/edit-post' ).hasMetaBoxes(); | ||
|
|
||
| // Save metaboxes on save completion when past save wasn't an autosave. | ||
| // Save metaboxes on save completion, except for autosaves that are not a post preview. | ||
| const shouldTriggerMetaboxesSave = ( | ||
| hasActiveMetaBoxes && | ||
| wasSavingPost && | ||
| ! wasAutosavingPost && | ||
| ! isSavingPost && | ||
| ! isAutosavingPost | ||
| hasActiveMetaBoxes && ( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope it works 🙏 😅 |
||
| ( wasSavingPost && ! isSavingPost && ! wasAutosavingPost ) || | ||
| ( wasAutosavingPost && wasPreviewingPost && ! isPreviewingPost ) | ||
| ) | ||
| ); | ||
|
|
||
| // Save current state for next inspection. | ||
| wasSavingPost = isSavingPost; | ||
| wasAutosavingPost = isAutosavingPost; | ||
| wasPreviewingPost = isPreviewingPost; | ||
|
|
||
| if ( shouldTriggerMetaboxesSave ) { | ||
| store.dispatch( requestMetaBoxUpdates() ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,10 +99,6 @@ export class PostPreviewButton extends Component { | |
| // unintentional forceful redirects. | ||
| if ( previewLink && ! prevProps.previewLink ) { | ||
| this.setPreviewWindowLink( previewLink ); | ||
|
|
||
| // Once popup redirect is evaluated, even if already closed, delete | ||
| // reference to avoid later assignment of location in post update. | ||
| delete this.previewWindow; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aduth I removed this to be able to refresh the window without an autosave (once the metaboxes finish saving). I'm not aware of any downsides but let me know if I missed something. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -152,7 +148,7 @@ export class PostPreviewButton extends Component { | |
|
|
||
| // Request an autosave. This happens asynchronously and causes the component | ||
| // to update when finished. | ||
| this.props.autosave(); | ||
| this.props.autosave( { isPreview: true } ); | ||
|
|
||
| // Display a 'Generating preview' message in the Preview tab while we wait for the | ||
| // autosave to finish. | ||
|
|
@@ -192,7 +188,7 @@ export class PostPreviewButton extends Component { | |
| } | ||
|
|
||
| export default compose( [ | ||
| withSelect( ( select ) => { | ||
| withSelect( ( select, { forcePreviewLink, forceIsAutosaveable } ) => { | ||
| const { | ||
| getCurrentPostId, | ||
| getCurrentPostAttribute, | ||
|
|
@@ -215,9 +211,9 @@ export default compose( [ | |
| return { | ||
| postId: getCurrentPostId(), | ||
| currentPostLink: getCurrentPostAttribute( 'link' ), | ||
| previewLink, | ||
| previewLink: forcePreviewLink !== undefined ? forcePreviewLink : getAutosaveAttribute( 'preview_link' ), | ||
| isSaveable: isEditedPostSaveable(), | ||
| isAutosaveable: isEditedPostAutosaveable(), | ||
| isAutosaveable: forceIsAutosaveable || isEditedPostAutosaveable(), | ||
| isViewable: get( postType, [ 'viewable' ], false ), | ||
| }; | ||
| } ), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1518,7 +1518,18 @@ export function didPostSaveRequestFail( state ) { | |
| * @return {boolean} Whether the post is autosaving. | ||
| */ | ||
| export function isAutosavingPost( state ) { | ||
| return isSavingPost( state ) && state.saving.isAutosave; | ||
| return isSavingPost( state ) && !! state.saving.options.isAutosave; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if the post is being previewed, or false otherwise. | ||
| * | ||
| * @param {Object} state Global application state. | ||
| * | ||
| * @return {boolean} Whether the post is being previewed. | ||
| */ | ||
| export function isPreviewingPost( state ) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's slightly weird that preview would have this side-effect but it wouldn't be evident from selector name. Though I guess it matches the expectation of the action itself. |
||
| return isSavingPost( state ) && !! state.saving.options.isPreview; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I don't think we need
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. it's better to have it to ensure the return value is always a boolean. |
||
| } | ||
|
|
||
| /** | ||
|
|
||
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.
It could be refactored to:
and then we only would need to update the code later in the execution flow to:
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.
it can't because we're forcing the
nullvalue from outside. The inner component don't know what value to put unless we rename the prop something likeforceEmptyPreviewLink