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: Open new window if prior preview window closed #8446

Merged
merged 1 commit into from Aug 15, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Aug 3, 2018

This pull request seeks to resolve an issue where the Preview button will do nothing in some circumstances where a preview had been shown earlier but closed.

Implementation notes:

The user flow here is very specific because of how the componentDidUpdate unsets the preview window, thus allowing new windows to be opened if either the previous tab was initially closed or left open, but not if closed after left open and previewed multiple times.

As a follow-up task, I think the save process should be made more durable by restoring the behavior of componentDidUpdate operating on isAutosaving. It used to be this way, but was made to be more generic. Making it specific to the autosaving flow enables handling error cases as well, rather than letting the failed preview window linger open. It also prevents needing awkwardnesses around deleting the previewWindow instance variable, which is only needed because we're attempting to be unaware of the autosave flow.

End-to-end tests are planned to be written, but cannot be done so currently with failing Docker builds (#8418).

Testing instructions:

Verify that a preview is shown in the following set of steps:

  1. Navigate to Posts > Add New
  2. Enter a title
  3. Press Preview
  4. Leaving the preview tab open, return to the editor tab
  5. Press Preview
  6. Close the preview tab
  7. Press Preview (on the editor tab)

Ensure that a preview is shown.

closes #8996

@@ -64,7 +64,7 @@ export class PostPreviewButton extends Component {
// Open a popup, BUT: Set it to a blank page until save completes. This
// is necessary because popups can only be opened in response to user
// interaction (click), but we must still wait for the post to save.
if ( ! this.previewWindow ) {
if ( ! this.previewWindow || this.previewWindow.closed ) {

This comment has been minimized.

@youknowriad

youknowriad Aug 15, 2018

Contributor

Do you think we should add the this.previewWindow.focus() in all cases like I did in #9015

@youknowriad

youknowriad Aug 15, 2018

Contributor

Do you think we should add the this.previewWindow.focus() in all cases like I did in #9015

This comment has been minimized.

@aduth

aduth Aug 15, 2018

Member

Discussed at #9015 as a repurposing of that branch.

@aduth

aduth Aug 15, 2018

Member

Discussed at #9015 as a repurposing of that branch.

@youknowriad

LGTM 👍

@aduth aduth merged commit a0e0847 into master Aug 15, 2018

2 checks passed

codecov/project 50.84% (+0%) compared to 426f484
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the fix/preview-window-closed branch Aug 15, 2018

@mtias mtias added this to the 3.6 milestone Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment