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

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 20, 2018

closes #12047
fixes #9151

This is a follow-up to #12037 (comment) and it builds on top of some changes in #11409

This PR changes the behavior of the preview button. It now performs a full draft save (on non published posts) before previewing the post. This matches Core behavior. (classic editor)

Testing instructions

  • Create a post
  • Set a featured image
  • Click preview
  • You should see the post with the featured image

@youknowriad youknowriad self-assigned this Nov 20, 2018
@youknowriad youknowriad requested review from adamsilverstein and a team November 20, 2018 10:52
*
* @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.

@gziolo gziolo added this to the 4.5 milestone Nov 20, 2018
@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Saving Related to saving functionality Backwards Compatibility Issues or PRs that impact backwards compatability labels Nov 20, 2018
@youknowriad youknowriad changed the base branch from fix/save-meta-on-preview to master November 20, 2018 11:41
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

@gziolo
Copy link
Member

gziolo commented Nov 20, 2018

FAIL test/e2e/specs/preview.test.js
● Preview › Should open a preview window for a new post
expect(received).toBe(expected) // Object.is equality
Expected: "http://localhost:8889/?p=144&preview=true"
Received: "http://localhost:8889/?p=144"
73 |
74 | let expectedPreviewURL = getUrl( '', ?p=${ postId }&preview=true );
> 75 | expect( previewPage.url() ).toBe( expectedPreviewURL );
| ^
76 |
77 | // Title in preview should match input.
78 | let previewTitle = await previewPage.$

There is one e2e test to fix and we should be good to go. In my testing, everything works as expected.

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.


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.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It works properly in my testing. I would appreciate someone else doing a sanity check as I'm not super familiar with the preview logic. I performed my tests comparing how it works in Classic editor with what this PR proposes.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Saving Related to saving functionality [Type] Bug An existing feature does not function as intended
Projects
None yet
3 participants