Skip to content
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

Perform a complete draft save on preview #12097

Merged
merged 19 commits into from
Nov 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions docs/data/data-core-editor.md
Original file line number Diff line number Diff line change
Expand Up @@ -1083,6 +1083,18 @@ Returns true if the post is being previewed, or false otherwise.

Whether the post is being previewed.

### getEditedPostPreviewLink

Returns the post preview link

*Parameters*

* state: Global application state.

*Returns*

Preview Link.

### getSuggestedPostFormat

Returns a suggested post format for the current post, inferred only if there
Expand Down
20 changes: 10 additions & 10 deletions packages/editor/src/components/post-preview-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { __, _x } from '@wordpress/i18n';
import { withSelect, withDispatch } from '@wordpress/data';
import { DotTip } from '@wordpress/nux';
import { ifCondition, compose } from '@wordpress/compose';
import { addQueryArgs } from '@wordpress/url';

function writeInterstitialMessage( targetDocument ) {
let markup = renderToString(
Expand Down Expand Up @@ -148,7 +147,11 @@ export class PostPreviewButton extends Component {

// Request an autosave. This happens asynchronously and causes the component
// to update when finished.
this.props.autosave( { isPreview: true } );
if ( this.props.isDraft ) {
this.props.savePost( { isPreview: true } );
} else {
this.props.autosave( { isPreview: true } );
}

// Display a 'Generating preview' message in the Preview tab while we wait for the
// autosave to finish.
Expand Down Expand Up @@ -192,33 +195,30 @@ export default compose( [
const {
getCurrentPostId,
getCurrentPostAttribute,
getAutosaveAttribute,
getEditedPostAttribute,
isEditedPostSaveable,
isEditedPostAutosaveable,
getEditedPostPreviewLink,
} = select( 'core/editor' );
const {
getPostType,
} = select( 'core' );

let previewLink = getAutosaveAttribute( 'preview_link' );
const featuredImageId = getEditedPostAttribute( 'featured_media' );
if ( previewLink && featuredImageId ) {
previewLink = addQueryArgs( previewLink, { _thumbnail_id: featuredImageId } );
}

const previewLink = getEditedPostPreviewLink();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previewLink can be inlined again.

const postType = getPostType( getEditedPostAttribute( 'type' ) );
return {
postId: getCurrentPostId(),
currentPostLink: getCurrentPostAttribute( 'link' ),
previewLink: forcePreviewLink !== undefined ? forcePreviewLink : getAutosaveAttribute( 'preview_link' ),
previewLink: forcePreviewLink !== undefined ? forcePreviewLink : previewLink,
isSaveable: isEditedPostSaveable(),
isAutosaveable: forceIsAutosaveable || isEditedPostAutosaveable(),
isViewable: get( postType, [ 'viewable' ], false ),
isDraft: [ 'draft', 'auto-draft' ].indexOf( getEditedPostAttribute( 'status' ) ) !== -1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a possible candidate for a selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a bunch of isEditedPostSomething, this could be isEditedPostDraft but again is auto-draft a draft.

But overall agreed, for now it's specific to this component so maybe fine to leave it as is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add it later.

};
} ),
withDispatch( ( dispatch ) => ( {
autosave: dispatch( 'core/editor' ).autosave,
savePost: dispatch( 'core/editor' ).savePost,
} ) ),
ifCondition( ( { isViewable } ) => isViewable ),
] )( PostPreviewButton );
24 changes: 21 additions & 3 deletions packages/editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
*/
import { isReusableBlock } from '@wordpress/blocks';
import { combineReducers } from '@wordpress/data';
import { addQueryArgs } from '@wordpress/url';

/**
* Internal dependencies
Expand Down Expand Up @@ -1203,13 +1204,29 @@ export function autosave( state = null, action ) {
title,
excerpt,
content,
preview_link: post.preview_link,
};
}

return state;
}

/**
* Reducer returning the poost preview link
*
* @param {string?} state The preview link
* @param {Object} action Dispatched action.
*
* @return {string?} Updated state.
*/
export function previewLink( state = null, action ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preview link is tracked separately as it's not directly related to autodrafts anymore.

switch ( action.type ) {
case 'REQUEST_POST_UPDATE_SUCCESS':
return action.post.preview_link || addQueryArgs( action.post.link, { preview: true } );

case 'REQUEST_POST_UPDATE_START':
// Invalidate known preview link when autosave starts.
if ( state && action.options.isAutosave ) {
return omit( state, 'preview_link' );
if ( state && action.options.isPreview ) {
return null;
}
break;
}
Expand All @@ -1233,6 +1250,7 @@ export default optimist( combineReducers( {
reusableBlocks,
template,
autosave,
previewLink,
settings,
postSavingLock,
} ) );
18 changes: 18 additions & 0 deletions packages/editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
} from '@wordpress/blocks';
import { isInTheFuture, getDate } from '@wordpress/date';
import { removep } from '@wordpress/autop';
import { addQueryArgs } from '@wordpress/url';

/**
* Dependencies
Expand Down Expand Up @@ -1532,6 +1533,23 @@ export function isPreviewingPost( state ) {
return isSavingPost( state ) && !! state.saving.options.isPreview;
}

/**
* Returns the post preview link
*
* @param {Object} state Global application state.
*
* @return {string?} Preview Link.
*/
export function getEditedPostPreviewLink( state ) {
const featuredImageId = getEditedPostAttribute( state, 'featured_media' );
const previewLink = state.previewLink;
if ( previewLink && featuredImageId ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add also a unit test, can be a follow-up PR

return addQueryArgs( previewLink, { _thumbnail_id: featuredImageId } );
}

return previewLink;
}

/**
* Returns a suggested post format for the current post, inferred only if there
* is a single block within the post and it is of a type known to match a
Expand Down
2 changes: 0 additions & 2 deletions packages/editor/src/store/test/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2422,15 +2422,13 @@ describe( 'state', () => {
raw: 'The Excerpt',
},
status: 'draft',
preview_link: 'https://wordpress.org/?p=1&preview=true',
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
},
} );

expect( state ).toEqual( {
title: 'The Title',
content: 'The Content',
excerpt: 'The Excerpt',
preview_link: 'https://wordpress.org/?p=1&preview=true',
} );
} );
} );
Expand Down
12 changes: 1 addition & 11 deletions test/e2e/specs/preview.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ describe( 'Preview', () => {
return window.location.search.match( /[\?&]post=(\d+)/ );
} ) ).jsonValue();

let expectedPreviewURL = getUrl( '', `?p=${ postId }&preview=true` );
const expectedPreviewURL = getUrl( '', `?p=${ postId }&preview=true` );
expect( previewPage.url() ).toBe( expectedPreviewURL );

// Title in preview should match input.
Expand All @@ -97,16 +97,6 @@ describe( 'Preview', () => {
// Preview for published post (no unsaved changes) directs to canonical URL for post.
await editorPage.bringToFront();
await publishPost();
// Wait until the publish panel is closed
await Promise.all( [
editorPage.waitForFunction( () => ! document.querySelector( '.editor-post-publish-panel' ) ),
editorPage.click( '.editor-post-publish-panel__header button' ),
] );
expectedPreviewURL = await editorPage.$eval( '.components-notice.is-success a', ( node ) => node.href );

await editorPage.bringToFront();
await waitForPreviewNavigation( previewPage );
expect( previewPage.url() ).toBe( expectedPreviewURL );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this part because in my testing even if the url is a preview url and not the final url, it works as intended.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works this way in classic, so maybe we should keep it with this param.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember why this part is here, @aduth should confirm whether we can remove it. I remember that this test was very fragile in the past.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea was that if there is nothing to save and we have a known preview URL, we don't save, we go straight to the preview URL.

I don't know that it's necessarily breaking to let the save go through, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that was my thinking, if the save request returns a URL, just use it whether there are dirtiness or not shouldn't affect it. and also restoring the old behavior was adding a bit of unnecessary complexity.


// Return to editor to change title.
await editorPage.bringToFront();
Expand Down