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

Preview: Enable published posts preview #7189

Merged
merged 13 commits into from Jun 11, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 6, 2018

Closes (when #7130 is merged): #7179
Closes (when #7130 is merged): #2544
Related: #6257, #6882

Note: Merges to fix/autosave-preview (#7130) as base branch.
Note: As best I can tell, this branch is fully functional, but I have plans to include new end-to-end tests for various preview circumstances prior to merge.

This pull request seeks to allow the user to preview changes to a published post without first updating the post (i.e. the autosave revision preview).

In doing so, it...

To the latter point, I landed at the conclusion that preview_link only makes sense in the context of autsosaves on consideration of:

  • The post endpoints reflect the value of the canonical saved post. If there are no unsaved changes, the user should be directed to post.link as its preview.
  • It does not belong in the revisions endpoints because it is not the case that any revision can be previewed. A preview only occurs for the latest revision of a post (_set_preview, wp_get_post_autosave).

Related Trac tickets:

Thus, if there are no unsaved changes for a post, the Preview button is effectively a link to post.link. If there are unsaved changes, the autosave is triggered and once its responses' autosave.preview_link becomes accessible, it is used in the redirect (regardless of whether or not the autosave occurred as a full save as described in #7124).

@danielbachhuber
Copy link
Member

To the latter point, I landed at the conclusion that preview_link only makes sense in the context of autsosaves on consideration of:

This is an interesting idea. I hadn't considered it this way before, but it seems like a reasonable decision to me (particularly because I don't think core's existing implementation of previews is this well-considered).

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Couple small nits. I also ran into an issue where the preview doesn't reflect the title:

image

image

It's not immediately obvious to me why that discrepancy exists.

@@ -2298,13 +2298,15 @@ describe( 'state', () => {
raw: 'The Excerpt',
},
status: 'draft',
preview_link: 'https://wordpress.org/?p=1',
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should this include &preview=true for clarity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nit: Should this include &preview=true for clarity?

Yep, sounds good.


if ( ! empty( $schema['properties']['preview_link'] ) ) {
$response->data['preview_link'] = get_preview_post_link( $post->post_parent, array(
'preview_id' => $post->post_parent,
Copy link
Member

Choose a reason for hiding this comment

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

Is preview_id supposed to be $post->post_parent? This ends up as 0 on a draft post:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Is preview_id supposed to be $post->post_parent? This ends up as 0 on a draft post:

Good call. We might consider using something like wp_is_post_autosave to vary the logic. Though in this context since we assume the $post is the result of either an autosave (get_item) or generated by an autosave intent, maybe just testing its type $post->post_type === 'revision'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm the -autosave post name suffix checking in wp_is_post_autosave makes me think it might be better to rely on that. Guessing it might be possible for a non-autosave revision to be attached to a post parent?

@aduth
Copy link
Member Author

aduth commented Jun 7, 2018

Couple small nits. I also ran into an issue where the preview doesn't reflect the title:

I can reproduce this, and it seems to affect where we're previewing a post we'd already previewed once before in the same session:

  1. Navigate to Posts > Add New
  2. Enter a title
  3. Hit preview
  4. Return to editor tab
  5. Change title
  6. Hit preview

Expected: Preview is shown with updated title
Actual: Preview reflects original title.

The problem is that the second time we preview, we already have a preview_link available because we reset the autosave after the first preview. I think we either need to:

  • Clear the preview_link before an autosave starts so it isn't considered for the popup window
  • Consider change detection and invalidate the preview link as soon as the user makes an edit
  • Force the autosave and interstitial screen if the post is dirty, even if a preview link is already available
    • Might be the simplest approach for now. Technically this is wasted for repeatedly hitting "Preview" on a published post, since it remains dirty after an autosave.
    • Effectively a revert of 7658b07

@aduth
Copy link
Member Author

aduth commented Jun 7, 2018

Another complication: For repeated previews of a draft post, even if I make changes, the preview URL will never change (e.g. ?p=123&preview=true), so it's hard to know that the preview is ready without either (re-)integrating awareness of the save process, or invalidating the preview link at some point (e.g. on edit or pre-autosave)

aduth added 9 commits June 7, 2018 14:34
Was not reliably triggered when redirect occurs by setPreviewWindowLink, possibly because document.open triggered by document.write erased window handlers.
Allows use in handlers to assume options present
Preview link is no longer valid until autosave completes, by which UI can infer availability of preview as being ready to view.
@adamsilverstein
Copy link
Member

Thanks for working on this @aduth - I'll give it a test and review as soon as possible!

// An autosave may be processed by the server as a regular save
// when its update is requested by the author and the post was
// draft or auto-draft.
const isRevision = newPost.id !== post.id;

// Thus, the following behaviors occur:
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that these removed changes were introduced as part of the base branch to which this merges (#7130) so there is effectively no difference between this callback and what's in master at the moment.

@aduth
Copy link
Member Author

aduth commented Jun 11, 2018

Going to merge this into the fix/autosave-preview branch (#7130) it has assigned as base, so as to be able to work on these related changes in one place.

@aduth aduth merged commit 111fb73 into fix/autosave-preview Jun 11, 2018
@aduth aduth deleted the add/publish-preview branch June 11, 2018 15:43
@aduth
Copy link
Member Author

aduth commented Jun 11, 2018

Continued at #7130

@mtias mtias added this to the 3.1 milestone Jun 20, 2018
aduth added a commit that referenced this pull request Jun 20, 2018
* REST API: Move preview_link to autosaves controller

* Post Preview: Redirect to preview by autosave property availability

* Preview Button: Use preview link as href if available

* Preview Button: Move window bind to after write

Was not reliably triggered when redirect occurs by setPreviewWindowLink, possibly because document.open triggered by document.write erased window handlers.

* Preview Button: Always trigger autosave when dirty

* Preview Button: Consider preview ready by change in value

* State: Set default value for savePost options

Allows use in handlers to assume options present

* Store: Clarify preview link with query parameter

* REST API: Set preview link by parent only if autosave

* Store: Invalid autosave preview link on save intent

Preview link is no longer valid until autosave completes, by which UI can infer availability of preview as being ready to view.

* Testing: Add preview E2E tests

* Testing: Fix copypasta

* Preview: Use correct location property for URL comparison
aduth added a commit that referenced this pull request Jun 20, 2018
…7130)

* State: Trigger autosave as standard save for draft by current user

* Testing: Update E2E tests for expected autosave conditions

* Testing: Wait for animation to avoid cursor chasing button

* State: Distinguish save on effective autosave

Change detection reset for known-to-be-draft, pick from non-effective autosave for resetting post

* Testing: Ensure autosave is identified as autosave

Passed previously because when autosaving, class is assigned with _both_ `is-saving` and `is-autosaving`. The superset of classes is what what should be expected.

* State: Update post in autosave as assumed full save

* Preview: Enable published posts preview (#7189)

* REST API: Move preview_link to autosaves controller

* Post Preview: Redirect to preview by autosave property availability

* Preview Button: Use preview link as href if available

* Preview Button: Move window bind to after write

Was not reliably triggered when redirect occurs by setPreviewWindowLink, possibly because document.open triggered by document.write erased window handlers.

* Preview Button: Always trigger autosave when dirty

* Preview Button: Consider preview ready by change in value

* State: Set default value for savePost options

Allows use in handlers to assume options present

* Store: Clarify preview link with query parameter

* REST API: Set preview link by parent only if autosave

* Store: Invalid autosave preview link on save intent

Preview link is no longer valid until autosave completes, by which UI can infer availability of preview as being ready to view.

* Testing: Add preview E2E tests

* Testing: Fix copypasta

* Preview: Use correct location property for URL comparison

* State: Partially pick saved fields for autosave

Must dirty artificially once save completes if there were edits not included in the payload.

* Testing: Update published change detection test to use publishPost utility

* REST API: Fix code styling (aligned assignment operator)

* Autosave: Remove redundant draft transition of reset autosave

Now reflected in committed updatePost edit of status to draft in save effect for auto-draft (by current user).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants