Editor: Update behavior of notice and preview action

[jeremeylduvall]

This is a first attempt at fixing #12829. It feels a bit hacky tbh. Right now, the "Preview" link in the Editor and the "View" link have the same behavior. This works in most cases, but in specific instances, it produces unexpected behavior. For example:

I have a published post. I make changes to the post, but I haven't updated the post yet. I would expect the "View" link in the notice to show me the saved version of the post (without my new changes). I would expect the "Preview" link to show me the new changes as well. Right now, you would see the new changes in both instances.

To decipher when we should show a preview versus the published post, I introduced a new state item previewAction that changes according to which action is taken by the user.

While testing this out, I also noticed that when we modify the ExternalUrl prop in editor-preview/index.jsx, we're stripping out the preview component. This leads to some counterintuitive behavior. For example, try the following:

1. Start a new post and enter some content. Publish the post.
2. Enter some new content in the post, but do not update the post.
3. Click the "Preview" link at the top of the Editor. The iframe will show your changes as expected.
4. Click the external icon on the iframe. The resulting window will not show your changes.

I would expect the iframe and the external icon URL to show the same content. If we leave in the preview piece to the URL, this behavior is fixed. cc @ehg as it looks like you removed it in #5550. I'm not quite clear on the ramifications of leaving preview. I didn't see any adverse side effects.

To test

1. Create a new post.
2. Type in some content and use the Preview link at the top of the editor. Verify that you see the same content as currently in the editor.
3. Publish the post. The View Post link should show the same content.
4. Leave the notice in place. Add some new content to the editor, but don't update it.
5. Click on View Post. You should not see the new content that you haven't updated/saved yet. Click on Preview at the top of the editor. You should see the new content since it's a post preview.

GIF

Here's a GIF of the various flows.

[matticbot]

Test live: https://calypso.live/?branch=fix/12829-preview-link-editor

[aduth]

Rather than applying as state, we could pass the intended after-save action as an argument to onPreview. This way we don't need to manage the state lifecycle, while still being certain that the value is preserved after the save callback completes (idea of closure).

[ehg]

I'm not quite clear on the ramifications of leaving preview. I didn't see any adverse side effects.

This was an attempt to stop the pasting of links with generally noisy qargs by users. I guess the expected behaviour here would be to only include the preview qarg if the editor is in a 'dirty state'?

[jeremeylduvall]

@aduth thanks for the feedback! What do you think about this new approach? It no longer uses state. Instead, we're passing an argument to onPreview that will indicate which approach we should take.

[aduth]

I may have made an error in suggesting against state. Specifically, I think I overlooked how we're passing URLs to the EditorPreview in the render. In these cases, we'd need to guarantee that a rerender occurs when calling onPreview. Setting an instance property alone is not sufficient for this. Instead, we'd need to affect some change to the component's state, or incoming properties (or elsewhere that would incur this as a side effect, like changes to the post store). It may work as-is, but only because some other change is occurring that's causing a rerender, but relying on this could be prone to bugs in the future.

I don't recall exactly what your original implementation looked like, but I'm guessing assigning and reading from previewAction as state with setState and this.state would be preferable. That or trying to consolidate this.state.previewUrl to automatically reflect whether we're viewing or previewing the post. I don't know that the latter would be very easy to achieve, so it might be fine to have two separate properties in state (previewAction, previewUrl).

Sorry to flip-flop on my suggestions, and for my delays in revisiting this.

[jeremeylduvall]

Will take a look at this tomorrow. Setting the URL is indeed a bit of a wrinkle. I remember originally looking at:

That or trying to consolidate this.state.previewUrl to automatically reflect whether we're viewing or previewing the post

I forget exactly why I didn't go with that originally.

[jeremeylduvall]

I opted to re-add previewAction to state and modify the preview/view behavior using this.state.

Manipulating this.state.previewUrl could get confusing since taking this approach means that we would need to change previewUrl to the post URL at some points. I thought it made more sense to always have previewUrl reflect the preview and use this.state.post.URL when necessary.

This should work as originally described. I'll go ahead and flip it back to "Needs Review."

[jeremeylduvall]

Quick note: This comparison is helpful if the user does the following:

1. Cmd + Click on Preview to open a new tab.
2. Come back to the editor and click the Preview link like normal while the other tab is still open.

In that scenario, this.state.previewAction would still be set to preview so we're avoiding an extra setState call.

[aduth]

In that scenario, this.state.previewAction would still be set to preview so we're avoiding an extra setState call.

Hmm, kinda goes to highlight that letting the state be long-lived beyond the initial view/preview intent is perhaps a bit fragile. I don't really have a good alternative to suggest, but a little unnerving.

[jeremeylduvall]

Right - we could use a setTimeout to eventually revert the state back to null, but that still feels a bit hacky. Outside of closing the preview window, there isn't a great cue to reset the this.state.previewAction back to null :(

[aduth]

This seems to work well in my testing. Let me know what you think of my remarks, but I'm not opposed to moving forward with merging.

[aduth]

Since this ternary expression occurs four separate times, maybe it'd be worth moving to a helper function on the component?

getPreviewUrl() {
	const { previewAction, previewUrl, post } = this.state;
	if ( 'view' === previewAction ) {
		return post.URL;
	}

	return previewUrl;
}

[jeremeylduvall]

Good call. Ternary converted to helper function. getPreviewUrl introduces a tiny bit of shadowing since PostEditStore has a method with the same name. This naming makes the most sense and hopefully it's still straightforward enough.

[aduth]

In that scenario, this.state.previewAction would still be set to preview so we're avoiding an extra setState call.

Hmm, kinda goes to highlight that letting the state be long-lived beyond the initial view/preview intent is perhaps a bit fragile. I don't really have a good alternative to suggest, but a little unnerving.

[supernovia]

Just noting I ran into issues with this today. It's a bit awkward to have to tell folks they need to save before they can preview, so I'd love to see this fixed.

[matticbot]

@jeremeylduvall This PR needs a rebase

[aduth]

Looks good 👍