Add an 'AMP Preview' button to the block editor #3323

kienstra · 2019-09-23T03:21:13Z

Update: this has changed, please see more recent comments below

Like Pascal mentioned, this is mainly forked from Gutenberg's PostPreviewButton.

Maybe the preview button, or its location, could use improvement:

Option 2 looks really good, but I didn't see a way to implement that.

For the preview tab, this uses a fork of an AMP icon from amp.dev:

Closes #2934

Use the plugin pattern in the block-editor/ directory. Maybe the UI could be improved, especially the icon.

…'t selected There's no need to preview AMP if it's not enabled.

…t approach I'm not sure how the tests seemeingly passed before, though they're not passing now.

westonruter · 2019-09-23T04:05:01Z

assets/src/block-editor/plugins/amp-toggle.js

+			{
+				isEnabled && (
+					<PluginPostStatusInfo>
+						<AMPPreview />


This should only be displayed of the template mode is not Standard. It's only applicable to Transitional and Reader modes.

This now only displays if:

isEnabled && ! isStandardMode

assets/src/block-editor/components/amp-preview.js

westonruter · 2019-09-23T04:07:24Z

assets/src/block-editor/components/amp-preview.js

+
+		return {
+			postId: getCurrentPostId(),
+			currentPostLink: getCurrentPostAttribute( 'link' ),


This should get the query var added to it as well.

Sure, that's applied here

As Weston suggested, that URL should also have the AMP query var.

As a follow-up to adding 'ampSlug' to thie wp_localize_script() values, this tests that the right values are present.

These were mainly copied, so apply the correct types.

This still needs more testing. But so far, it looks like it's working. It inserts the button into the DOM on domReady, and in that component's componentDidUpdate(), it moves it back to the correct position if needed.

This allows the (non-AMP) preview button to align with the end of the .edit-post-sidebar.

… post On creating a new post and making the first edit, the 'Preview AMP' button appeared out of place.

…view [WIP] Try manipulating the DOM to add the 'Preview AMP' button

The conflict was in assets/src/block-editor/index.js

There are 2 tests that failed, apparently from dependencies not being added to the expected dependencies.

There's already a method in that for isStandardMode(), and add 2 new methods. This addresses a console error from ampBlockEditor not being defined.

Delete AMP_PREVIEW_BUTTON_WRAPPER_ID, as there's no need to store it in constants.js. Simply define it as a const in renderPreviewButton(), the only function where it's used.

kienstra · 2019-10-22T18:06:35Z

tests/php/validation/test-class-amp-validation-manager.php

 			'wp-polyfill',
+			'wp-url',


These are probably from the dependencies in amp-preview.js.

Using that version of Core, the icons were too small.

Clearing my stale review

Before, there was a console error from an <svg> property.

Add a param to that, so that it's easier to test. It'll accept the component that it renders as a parameter.

This is just and if block, and it's probably not necessary to have **.

Still, these might be needed, so I'll have to look at this more.

This was dependent on whether the AMP Preview button was present, so it should be good to not need this.

…view This is needed for WP before 5.3, or 5.2.4 with Gutenberg active. Without this fix, the 'Preview AMP' button can be 1px taller than the non-AMP preview button.

That's only needed at the end of the test. Also, change a tipId to have 'amp' instead of 'core'.

It looks like these aren't needed. Also, clarify a comment.

kienstra · 2019-10-27T21:37:34Z

assets/src/block-editor/components/amp-preview.js

+		</style>
+	`;
+
+	targetDocument.write( markup );


In the <PostPreviewButton> in Gutenberg that this is forked from, there's a filter for markup:

markup = applyFilters( 'editor.PostPreview.interstitialMarkup', markup );

I decided not to add one here, but feel free to comment if you'd like one.

This inserts the AMP preview button before that nextSibling, so it's probably best to exit if it doesn't exist.

The function runs on domReady, so it should only run 1 time, and the component shouldn't be rendered into the DOM when it runs.

kienstra · 2019-10-27T22:04:36Z

assets/images/amp-black-icon.svg

@@ -0,0 +1,4 @@
+<svg xmlns="http://www.w3.org/2000/svg">


Copied and modified from an .svg file from the 'Logos' link in the bottom right of https://amp.dev/

The moveButton function comes before openPreviewWindow in the class, so move it above it.

kienstra · 2019-10-27T23:16:14Z

Without Gutenberg active

With Gutenberg active

kienstra · 2019-10-27T23:31:58Z

Request For Review

Hi @westonruter and @swissspidy,
Hope you had a great weekend.

Could you please review this when you have a chance?

It adds an AMP preview button, when in Transitional or Reader mode:

The button is forked from the Gutenberg <PostPreviewButton> component, and is rendered into the DOM on domReady.

Thanks, and no pressure if you'd like to focus on PRs needed for 1.4.

swissspidy

Generally works fine.

One thing I noticed:

Click on AMP Preview button
AMP Preview is opened
Click on normal Preview button
AMP Preview tab is reloaded, but still shows AMP version

kienstra · 2019-11-08T18:11:29Z

Ah, good point. I'll work on that.

There was a failed Travis build, with ESLint issues for the lack of a trailing comma.

…P preview As Pascal mentioned, on opening an AMP Preview, clicking the non-AMP Preview button reloads the AMP preview. And vice versa.

kienstra · 2019-11-08T22:54:21Z

a2e18e7 fixed the issue of the non-AMP 'Preview' link reloading the AMP preview, at least in my local.

…-editor-amp-preview * 'develop' of github.com:ampproject/amp-wp: (30 commits) Fix identifying sources for validation errors coming child themes (#3708) Fix failing E2E tests (#3707) Remove amp_validate query var from Validated URL 'View' row action Re-factor get_html_attribute_pattern as match_element_attributes Quote variables added to regex pattern Escape instances of unescapeed output in AMP settings screen code (#3703) Replace incorrect usage of esc_url() with esc_url_raw() Remove empty alt attributes Add object-fit=contain to amp-youtube placeholder image Prevent copying empty title/alt in WP<5.2 Extract HTML attribute parsing logic into base class, and use for YouTube embeds Update doc comment for `get_video_id_from_url` Merge separate regexes used to retrieve props into one Replace ambiguous `$fallback` with `$fallback_for_expected` Refactor `get_video_id_from_url` function Replace the fallback with an image placeholder for the iframe Remove todo HTML entity encoding will be handled by the DOM instead Bump `oembed` function deprecation version to 1.5.0 Bump develop to v1.5.0-alpha ...

westonruter · 2019-11-11T05:34:32Z

assets/src/block-editor/components/amp-preview.js

+ * @see https://github.com/WordPress/gutenberg/blob/95e769df1f82f6b0ef587d81af65dd2f48cd1c38/packages/editor/src/components/post-preview-button/index.js#L17
+ * @param {Object} targetDocument The target document.
+ */
+function writeInterstitialMessage( targetDocument ) {


This is a nice touch.

assets/src/block-editor/components/amp-preview.js

westonruter · 2019-11-11T05:41:01Z

assets/src/block-editor/components/amp-preview.js

+	/**
+	 * Opens the preview window.
+	 *
+	 * @param {Object} event The DOM event.
+	 */
+	openPreviewWindow( event ) {
+		// Our Preview button has its 'href' and 'target' set correctly for a11y
+		// purposes. Unfortunately, though, we can't rely on the default 'click'
+		// handler since sometimes it incorrectly opens a new tab instead of reusing
+		// the existing one.
+		// https://github.com/WordPress/gutenberg/pull/8330
+		event.preventDefault();
+
+		// Open up a Preview tab if needed. This is where we'll show the preview.
+		if ( ! this.previewWindow || this.previewWindow.closed ) {
+			this.previewWindow = window.open( '', this.getWindowTarget() );
+		}
+
+		// Focus the Preview tab. This might not do anything, depending on the browser's
+		// and user's preferences.
+		// https://html.spec.whatwg.org/multipage/interaction.html#dom-window-focus
+		this.previewWindow.focus();
+
+		// If we don't need to autosave the post before previewing, then we simply
+		// load the Preview URL in the Preview tab.
+		if ( ! this.props.isAutosaveable ) {
+			this.setPreviewWindowLink( event.target.href );
+			return;
+		}
+
+		// Request an autosave. This happens asynchronously and causes the component
+		// to update when finished.
+		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.
+		writeInterstitialMessage( this.previewWindow.document );
+	}


Since this and setPreviewWindowLink are unchanged from core, could the core function not be called directly here?

For example, this function could be:

import { PostPreviewButton } from '@wordpress/editor'; // ... openPreviewWindow( event ) { PostPreviewButton.prototype.openPreviewWindow.call( this, event ) }

Alas, this doesn't seem to work. It seems compose prevents accessing the underlying functions on the prototype.

westonruter · 2019-11-11T06:00:27Z

I'm a bit nervous that the underlying Gutenberg PostPreviewButton will diverge from what has been copied into the plugin here, and that we'll need to maintain it as it evolves (or doesn't) change in core. But this works well now and we'll have to monitor during the 1.5 release cycle to see if any churn causes problems.

kienstra · 2019-11-11T17:16:45Z

That's a good point. This definitely comes with technical debt.

Add an 'AMP Preview' button to the block editor

4558a05

Use the plugin pattern in the block-editor/ directory. Maybe the UI could be improved, especially the icon.

googlebot added the cla: yes Signed the Google CLA label Sep 23, 2019

kienstra changed the title ~~Add an 'AMP Preview' button to the block editor~~ [WIP] Add an 'AMP Preview' button to the block editor Sep 23, 2019

kienstra added 2 commits September 22, 2019 22:41

Don't display the 'AMP Preview' button if the 'Enable AMP' toggle isn…

d86f090

…'t selected There's no need to preview AMP if it's not enabled.

Address failed PHPUnit test, though I'm not positive this is the righ…

75e357c

…t approach I'm not sure how the tests seemeingly passed before, though they're not passing now.

westonruter previously requested changes Sep 23, 2019

View reviewed changes

kienstra added 2 commits September 22, 2019 23:30

Use the AMP slug from amp_get_slug(), apply query var to currentPostLink

7a574ef

As Weston suggested, that URL should also have the AMP query var.

Test that the proper values are localized

cc1bedb

As a follow-up to adding 'ampSlug' to thie wp_localize_script() values, this tests that the right values are present.

kienstra changed the title ~~[WIP] Add an 'AMP Preview' button to the block editor~~ Add an 'AMP Preview' button to the block editor Sep 23, 2019

kienstra mentioned this pull request Sep 23, 2019

How can I preview AMP pages in Gutenberg? #2934

Closed

kienstra added 4 commits September 23, 2019 02:01

Correct the <AMPPreview> propTypes

98133f6

These were mainly copied, so apply the correct types.

Experiment with manipulating the DOM

f80dd86

Merge branch 'develop' into add/block-editor-amp-preview

cdc6e01

Try maniplating the DOM to add the 'Preview AMP' button

3459cbf

This still needs more testing. But so far, it looks like it's working. It inserts the button into the DOM on domReady, and in that component's componentDidUpdate(), it moves it back to the correct position if needed.

kienstra mentioned this pull request Sep 30, 2019

[WIP] Try manipulating the DOM to add the 'Preview AMP' button #3392

Merged

kienstra added 7 commits September 30, 2019 16:53

Adjust styling of the non-AMP 'Preview' button

097ba95

This allows the (non-AMP) preview button to align with the end of the .edit-post-sidebar.

Fix an issue where the 'Preview AMP' button was out of place on a new…

89bc7e8

… post On creating a new post and making the first edit, the 'Preview AMP' button appeared out of place.

Merge pull request #3392 from ampproject/try/dom-manipulation-amp-pre…

14df86f

…view [WIP] Try manipulating the DOM to add the 'Preview AMP' button

Merge in develop, resolve conflict

e46f3b6

The conflict was in assets/src/block-editor/index.js

Fix failing unit tests

c02a2d5

There are 2 tests that failed, apparently from dependencies not being added to the expected dependencies.

Use the 'amp/block-editor' store for the exported data

1dc42af

There's already a method in that for isStandardMode(), and add 2 new methods. This addresses a console error from ampBlockEditor not being defined.

Move a constant into a function, as it's only used there

038b078

Delete AMP_PREVIEW_BUTTON_WRAPPER_ID, as there's no need to store it in constants.js. Simply define it as a const in renderPreviewButton(), the only function where it's used.

kienstra commented Oct 22, 2019

View reviewed changes

Fix an issue with the icon display in WP 5.3

db963ac

Using that version of Core, the icons were too small.

kienstra added 5 commits October 26, 2019 14:02

Add an xmlns property to the <svg> so the build succeeds

788fb32

Before, there was a console error from an <svg> property.

Add a unit test for the helper renderPreviewButton

32057c7

Add a param to that, so that it's easier to test. It'll accept the component that it renders as a parameter.

Remove extra *,and use a simply one-line comment

c67f5b5

This is just and if block, and it's probably not necessary to have **.

Address failed unit tests by removing 2 expected dependencies

6631f44

Still, these might be needed, so I'll have to look at this more.

Remove styling for the (non-AMP) Preview button

885d2f2

This was dependent on whether the AMP Preview button was present, so it should be good to not need this.

kienstra added 3 commits October 26, 2019 19:04

Ensure the height of the 'Preview AMP' button matches the non-AMP pre…

463d75b

…view This is needed for WP before 5.3, or 5.2.4 with Gutenberg active. Without this fix, the 'Preview AMP' button can be 1px taller than the non-AMP preview button.

Remove an extra call in a unit test to renderPreviewButton()

32e3e31

That's only needed at the end of the test. Also, change a tipId to have 'amp' instead of 'core'.

Remove extra style rules for 'Preview AMP' button

1fc8bb6

It looks like these aren't needed. Also, clarify a comment.

kienstra commented Oct 27, 2019

View reviewed changes

kienstra added 2 commits October 27, 2019 15:55

Exit if the non-AMP 'Preview' button has no nextSibling

cc2a307

This inserts the AMP preview button before that nextSibling, so it's probably best to exit if it doesn't exist.

Remove a check for whether the component exists already

286ff79

The function runs on domReady, so it should only run 1 time, and the component shouldn't be rendered into the DOM when it runs.

kienstra commented Oct 27, 2019

View reviewed changes

Change the order of the .bind calls in the constructor

05994f0

The moveButton function comes before openPreviewWindow in the class, so move it above it.

kienstra requested a review from swissspidy October 27, 2019 23:32

Merge branch 'develop' into add/block-editor-amp-preview

627eca2

westonruter mentioned this pull request Oct 29, 2019

Add paired browsing interface #3656

Merged

10 tasks

Merge branch 'develop' into add/block-editor-amp-preview

1af4466

swissspidy reviewed Nov 8, 2019

View reviewed changes

kienstra added 2 commits November 8, 2019 16:45

Address ESLint issues

bc1b727

There was a failed Travis build, with ESLint issues for the lack of a trailing comma.

Fix an issue where clicking the non-AMP preview button reloads the AM…

a2e18e7

…P preview As Pascal mentioned, on opening an AMP Preview, clicking the non-AMP Preview button reloads the AMP preview. And vice versa.

westonruter added 2 commits November 10, 2019 21:26

Improve specificity of JS doc

0b1a5d3

westonruter reviewed Nov 11, 2019

View reviewed changes

westonruter approved these changes Nov 11, 2019

View reviewed changes

westonruter added this to the v1.5 milestone Nov 11, 2019

westonruter merged commit e9afd58 into develop Nov 11, 2019

westonruter deleted the add/block-editor-amp-preview branch November 11, 2019 06:22

westonruter mentioned this pull request Mar 8, 2020

AMP preview button looks off in WP 5.4-RC1 #4368

Closed

westonruter added the Changelogged label Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an 'AMP Preview' button to the block editor #3323

Add an 'AMP Preview' button to the block editor #3323

kienstra commented Sep 23, 2019 •

edited

Loading

westonruter Sep 23, 2019

kienstra Sep 23, 2019

kienstra Sep 23, 2019

westonruter Sep 23, 2019

kienstra Sep 23, 2019

kienstra Oct 22, 2019

kienstra Oct 27, 2019

kienstra Oct 27, 2019 •

edited

Loading

kienstra commented Oct 27, 2019

kienstra commented Oct 27, 2019

swissspidy left a comment

kienstra commented Nov 8, 2019

kienstra commented Nov 8, 2019

westonruter Nov 11, 2019

westonruter Nov 11, 2019

westonruter Nov 11, 2019

westonruter Nov 11, 2019

westonruter commented Nov 11, 2019

kienstra commented Nov 11, 2019

Add an 'AMP Preview' button to the block editor #3323

Add an 'AMP Preview' button to the block editor #3323

Conversation

kienstra commented Sep 23, 2019 • edited Loading

Update: this has changed, please see more recent comments below

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Oct 27, 2019 • edited Loading

Choose a reason for hiding this comment

kienstra commented Oct 27, 2019

Without Gutenberg active

With Gutenberg active

kienstra commented Oct 27, 2019

swissspidy left a comment

Choose a reason for hiding this comment

kienstra commented Nov 8, 2019

kienstra commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Nov 11, 2019

kienstra commented Nov 11, 2019

kienstra commented Sep 23, 2019 •

edited

Loading

kienstra Oct 27, 2019 •

edited

Loading