#796 Move AMP Customizer into main Customizer #819

kaitnyl · 2017-12-06T03:18:18Z

This PR removed the previous AMP Customizer that was fixed to one static post. It merges the AMP Customizer options into the main Customizer and provides a toggle to view all posts and pages if an AMP version is available.

Acceptance criteria is here.

Close #796

Make sure admin menu links to proper location.
Add selective refresh previewing of site title changes.
~~Add site icon previewing.~~

…ll posts and pages.

westonruter

@kaitnyl good progress!

One other thing that I realized we'll need to consider is the amp_customizer_is_enabled filter. When a plugin filters this, the intention is that the core AMP panel is not shown at all and none of our code gets included. I don't think we want this. Maybe we should then change the meaning of this filter then as a signal for whether or not just the AMP_Customizer_Design_Settings class gets loaded or not, and that is, whether the one AMP Design section gets registered. This will then allow us to keep the AMP panel and register our own “Settings” section to move the admin screen settings into the Customizer. This would seem to be faithful to the description of the filter: “If you're using a completely custom template”

westonruter · 2017-12-06T05:55:17Z

assets/js/amp-customize-controls.js

+	function isAmpUrl( url ) {
+		var urlParser = document.createElement( 'a' );
+		urlParser.href = url;
+		if ( /(^|\?|&)amp=1(?=&|$)/.test( urlParser.search ) ) {


We'll need to export the AMP_QUERY_VAR from PHP because a plugin can (for an unknown reason) rename the constant's value from "amp" to something else. Same goes for other instances of "amp" being referenced below.

westonruter · 2017-12-06T05:59:37Z

assets/js/amp-customize-controls.js

+			var ampEnabled = false,
+				headContent = $( api.previewer.preview.iframe ).contents().find( 'head' ).html();
+
+			if ( headContent.search( '<meta name="generator" content="AMP Plugin' ) >= 0 ) {


We'll need to change the approach here because if the Customizer preview window may not be on the same domain, and so attempting to access the DOM from the window will violate the cross-domain security policy.

So instead we need to have the Customizer preview app send a message to the controls app to indicate whether AMP is supported.

westonruter · 2017-12-06T06:00:30Z

assets/js/amp-customize-controls.js

+		// Notification tooltip for AMP toggle.
+		$( '.amp-toggle input' ).before( '<span class="tooltip">' +
+			'This page is not AMP compatible.<br>' +
+			'<a data-post="' + window.ampPost + '">Navigate to an AMP compatible page</a>' +


This needs be translated.

westonruter · 2017-12-06T06:01:58Z

assets/js/amp-customize-controls.js

+		'</label>' );
+
+		// Notification tooltip for AMP toggle.
+		$( '.amp-toggle input' ).before( '<span class="tooltip">' +


We should to use wp.template here instead of string concatenation. If the template is output via PHP we can also then make use of i18n in PHP to get the translated string.

westonruter · 2017-12-06T06:02:47Z

assets/js/amp-customize-controls.js

+		 * @param {bool} notification If tooltip notification should appear.
+		 * @return {void}
+		 */
+		function toggleControl( checked, disabled, notification ) {


Can this function take an object as its sole param instead of three positional params? I think that will make it easier to read.

This function can likely be removed altogether due to swapping to wp.customize.state 😄

westonruter · 2017-12-06T06:32:11Z

assets/js/amp-customize-controls.js

+
+		// Main control for toggling AMP preview.
+		$( '#customize-footer-actions' ).on( 'change', '.amp-toggle input', function( event ) {
+			ampToggle = $( this ).is( ':checked' );


I think the state here should be moved to wp.customize.state. For example, at the top of this file do:

wp.customize.state.add( 'ampEnabled', new wp.customize.Value() ); wp.customize.state.add( 'ampAvailable', new wp.customize.Value() );

The ampEnabled state can manage whether the toggle is on or off, whereas the ampAvailable state can manage whether or not AMP is available in the preview. Then it can be populated via messages from the preview via:

wp.customize.bind( 'ready', function( available ) { wp.customize.previewer.bind( 'ampAvailable', function( available ) { wp.customize.state( 'ampAvailable' ).set( available ); } ); } );

Then the disabled state of the toggle can be managed by the state like so:

wp.customize.state( 'ampAvailable' ).bind( function( available ) { $( '.amp-toggle input' ).prop( 'disabled', available ); } );

As noted above about sending messages from the preview about whether AMP is supported, there can then be a message receiver that then updates this state. In the preview it could do:

var ampAvailable = <?php echo wp_json_encode( is_singular() && post_supports_amp( get_queried_object() ) ); ?>; //... wp.customize.bind( 'preview-ready', function() { wp.customize.preview.send( 'ampAvailable', ampAvailable ); } );

Clicking on the checkbox can just change the ampEnabledstate, something like this:

$( '.amp-toggle input' ).on( 'click', function( event ) { event.preventDefault(); wp.customize.state( 'ampEnabled' ).set( ! wp.customize.state( 'ampEnabled' ).get() ); } );

And likewise the state of the button then can then in turn be managed by this state as well, as well as then setting the previewUrl:

wp.customize.state( 'ampEnabled' ).bind( function( enabled ) { $( '.amp-toggle input' ).prop( 'checked', enabled ); toggleAmpView(); } );

westonruter · 2017-12-06T06:32:31Z

assets/js/amp-customize-controls.js

+		} )( api.previewer.previewUrl.validate );
+
+		// Adding checkbox toggle before device selection.
+		$( '.devices-wrapper' ).before( '<label class="amp-toggle">' +


This can make use of a wp.template.

westonruter · 2017-12-06T06:33:15Z

assets/js/amp-customize-controls.js

+	 * @return {void}
+	 */
+	function panelReady( panel ) {
+		var ampToggle = false;


Remove this in favor of referring to wp.customize.state( 'ampEnabled' )

westonruter · 2017-12-06T06:33:53Z

assets/js/amp-customize-controls.js

+		 * @return {string} AMPified URL.
+		 */
+		function setCurrentAmpUrl( url ) {
+			if ( ! ampToggle && isAmpUrl( url ) ) {


The ampToggle here could be defined locally as:

var ampToggle = wp.customize.state( 'ampEnabled' ).get() && wp.customize.state( 'ampAvailable' ).get();

westonruter · 2017-12-06T06:36:09Z

assets/js/amp-customize-controls.js

+		 *
+		 * @return {void}
+		 */
+		function toggleAmpView() {


This might be better named updatePreviewUrl

…draft and previews) * Use query var in permalinks containing query vars, instead of endpoint slug. * Remove needless and unslightly 1 value. * Add tests.

…nslatable tooltip strings.

kaitnyl · 2017-12-07T14:03:42Z

@westonruter, with regards to your comment here, I think we should have a better way of identifying if the page can be AMPified. The current approach waits until the preview attempts to load the AMP'd page and then decides based on content that's returned to the preview (if the page has AMP metadata).

As the user navigates, we should detect if the current page can be AMPified and enable/disable the toggle before they attempt to use it. This would allow us to disable the AMP toggle before attemping to load the AMP URL in the preview and prevent that initial attempt to display the AMP version.

I'm thinking once the preview is ready/changes URLs, make an Ajax call that would enable or disable the bottom AMP toggle. The downside would be more backend communication as the user navigates, but upside is it would disable the instance where a user attempts to toggle on a page that isn't AMPified resulting in the preview trying to show it and then quickly changing back (2 refreshes).

Any thoughts/opinions on this?

… control UI.

westonruter · 2017-12-07T18:31:52Z

@kaitnyl I think the better way is outlined in #819 (comment). I don't see a need to do an additional Ajax request when the preview URL changes because the preview window is going to be issuing a request anyway, and when the preview loads, it can just send a message to the controls app to indicate whether or not AMP is available. If it is not available, then the toggle can be disabled. With the toggle disabled, then there would be no attempt to load the AMPed version and so no refreshes.

westonruter · 2017-12-07T18:34:48Z

assets/js/amp-customize-controls.js

 		urlParser.href = url;
-		if ( /(^|\?|&)amp=1(?=&|$)/.test( urlParser.search ) ) {
+		if ( regexParam.test( urlParser.search ) ) {


Instead of regexParam you can make use of wp.customize.utils.parseQueryString() and do something like:

if ( ! _.isUndefined( wp.customize.utils.parseQueryString( urlParser.search.substr( 1 ) )[ ampVars.query ] ) ) {

westonruter · 2017-12-07T18:37:22Z

assets/js/amp-customize-controls.js

-		urlParser.pathname = urlParser.pathname.replace( /\/amp\/?$/, '' );
-		urlParser.search = urlParser.search.replace( /(^|\?|&)amp=1/, '' );
+		urlParser.pathname = urlParser.pathname.replace( regexEndpoint, '' );
+		urlParser.search = urlParser.search.replace( regexParam, '' );


Per above, instead of regexParam I suggest doing something like:

if ( urlParser.search.length > 1 ) { var params = wp.customize.utils.parseQueryString( urlParser.search.substr( 1 ); delete params[ ampVars.query ]; urlParser.search = $.param( params ); }

This is similar to what is done in core: https://github.com/WordPress/wordpress-develop/blob/72b74509f3bfb71c884bf8cd9f580c026079498a/src/wp-admin/js/customize-controls.js#L6547-L6559

westonruter · 2017-12-07T18:43:05Z

includes/admin/class-amp-customizer.php

@@ -86,7 +87,14 @@ public function add_customizer_scripts() {
 			$footer  = true
 		);

-		wp_localize_script( 'amp-customizer', 'ampPost', amp_admin_get_preview_permalink() );
+		wp_localize_script( 'amp-customizer', 'ampVars', array(


Instead of adding a global variable, I suggest that the amp-customizer script export a boot method which we can then pass an inline script into. For example:

https://github.com/Automattic/amp-wp/blob/eeb502b28807c1be80e59937fab6112e406b204b/assets/js/amp-post-meta-box.js#L51-L59

https://github.com/Automattic/amp-wp/blob/eeb502b28807c1be80e59937fab6112e406b204b/includes/admin/class-amp-post-meta-box.php#L95-L111

This will avoid an additional global var, it will make the JS more encapsulated and easier to test, and wp_localize_script() has the nasty side effect of converting an scalar values to strings (which wouldn't apply here though).

westonruter · 2017-12-07T18:46:33Z

assets/js/amp-customize-controls.js

+
+		// Open AMP panel if mobile device selected.
+		api.previewedDevice.bind( function( device ) {
+			panel.expanded.set( 'mobile' === device );


Is this right? The AC says:

When a user selects to view the mobile version of a page that is AMP supported, the new toggle will automatically default to AMP. The user will be able to change it to Original if they wish to do so.

I think this means that ampEnabled should be set to true when switching to the mobile previewedDevice. I would think it could be annoying for the expanded panel to change when the user just changes the previewed device, as it would remove the controls context for where they were previously working.

…availability.

…enable AMP toggle if users selects mobile device.

…hin Customizer.

kaitnyl · 2017-12-08T22:23:34Z

@weston for your comment here regarding the amp_customizer_is_enabled filter, I've made changes for this within 0d1790b.

I've moved the filter to control only the UI portion of the AMP_Customizer_Design_Settings class. We still need some things in that class to load for the AMP preview in the Customizer to function correctly, and moving the filter here will ensure the AMP panel is still added (although empty and hidden).

* Include more Customizer preview logic from manager in AMP preview * Ensure scripts and styles are enqueued when preview opened for controls app; this ensures AMP validation can pass when using frontend preview link. * Restore previous add_preview_scripts method. * Include more logic behind * Add PHP doc. * Improve WPCS adherence. * Skip outputting Customizer preview JS if user cannot customize. * Use wp_print_footer_scripts()` instead of doing wp_footer.

Fix obsolete method references and improve placement of add_action() calls

… prevent fallback refreshes

…eview * Ensure that AMP Customizer preview JS is loaded in AMP and non-AMP mode so that the amp-status can be sent in each case. * Enable AMP when clicking toggle and navigating to an AMPed post. * Ensure password-protected posts are no returned by amp_admin_get_preview_permalink() * Add screen-reader-text CSS rule to AMP styles. * Move JS template vars into PHP-rendered template to simplify.

westonruter · 2017-12-09T07:57:33Z

@kaitnyl I think that the toggle needs some sort of hover and focus styling? Refer to Gutenberg.

Also use AMP_QUERY_VAR instaed of hard-coded 'amp' in amp_admin_get_preview_permalink()

ThierryA

Great work! Approved code wise from my perspective even though I posted a few good to have comments below.

I picked up that AC point 3 (below) does not work for me while testing this locally:

In the event a user navigates to a page that is not AMP enabled a notification will display within the AMP panel to inform the user that they should navigate to a URL that is AMP compatible.

ThierryA · 2017-12-11T17:26:28Z

assets/js/amp-customize-controls.js

+				$tooltip = $( '.tooltip', $( this ) ),
+				tooltipTimer = 5000;
+
+			if ( $input.prop( 'disabled' ) ) {


It would be good to prevent the tooltip from disappearing when a user's mouse is over the tooltip.

Or when keyboard focus is on the link in the tooltip.

ThierryA · 2017-12-11T17:29:17Z

assets/js/amp-customize-controls.js

+		api.previewer.bind( 'amp-status', setInitialAmpEnabledState );
+
+		// Persist the presence or lack of the amp=1 param when navigating in the preview,
+		// even if current page is not yet supported.


Small detail, this doesn't follow WP JS code standards (see here).

ThierryA · 2017-12-11T17:35:12Z

includes/settings/class-amp-customizer-design-settings.php

+	 */
+	public static function render_header_bar() {
+		if ( is_singular() ) {
+			amp_load_classes();


Make a note to remove all amp_load_classes() in #828.

westonruter · 2017-12-11T21:04:51Z

@ThierryA

I picked up that AC point 3 (below) does not work for me while testing this locally:

In the event a user navigates to a page that is not AMP enabled a notification will display within the AMP panel to inform the user that they should navigate to a URL that is AMP compatible.

This is currently indicated by the toggle being grayed out, and then you can click on the toggle to show the notification. Maybe the notification should be displayed in addition to the toggle being grayed?

…eature/796-amp-customizer

… unavailable URL

* Remove linebreak from tooltip for alignment with autosave restore notification; tweak copy. * Move functions from inside panel ready callback to component. * Improve styling in AMP when unloading page.

…compatible URL Improve styling of link in tooltip to improve color contrast

ThierryA · 2017-12-12T10:18:43Z

Great improvements @westonruter ❤️

Move AMP Customizer into main Customizer to expand usability across a…

2ea1341

…ll posts and pages.

kaitnyl changed the title ~~[WIP] #796 Move AMP Customizer into main Customizer~~ #796 Move AMP Customizer into main Customizer Dec 6, 2017

kaitnyl self-assigned this Dec 6, 2017

kaitnyl requested review from westonruter and ThierryA December 6, 2017 03:32

westonruter requested changes Dec 6, 2017

View reviewed changes

westonruter and others added 3 commits December 5, 2017 23:28

Fix amp_get_permalink() for permalinks containing query params (e.g. …

0885b48

…draft and previews) * Use query var in permalinks containing query vars, instead of endpoint slug. * Remove needless and unslightly 1 value. * Add tests.

Use AMP_QUERY_VAR in regex to identify/alter AMP URLs.

942a1fa

Make use of wp.template for adding HTML to Customizer and include tra…

9f34b14

…nslatable tooltip strings.

Move away from referencing input status and use wp.customize.state to…

d6159ab

… control UI.

westonruter reviewed Dec 7, 2017

View reviewed changes

kaitnyl added 4 commits December 8, 2017 15:31

Detect if page can be AMP'd from within Previewer and control toggle …

a716482

…availability.

Use alternative to regex to check if AMP is within query string, and …

65bf04a

…enable AMP toggle if users selects mobile device.

Avoid using wp_localize_script() to send data.

0679cb2

Move amp_customizer_is_enabled filter to only control UI elements wit…

0d1790b

…hin Customizer.

kaitnyl and others added 9 commits December 8, 2017 18:44

JSHint updates.

7e23ff2

Merge branch 'develop' into feature/796-amp-customizer

05e2a58

Fix eslint errors

4507a04

WIP: Fix styles

b2f0b4b

Fix retrieval of customized settings when Customizer is not bootstrapped

c349320

Add selective refresh of header-bar to customizer preview

df26de5

Fix obsolete method references and improve placement of add_action() calls

Add partial for footer; do wptexturize on title in header and footer;…

e0c61f5

… prevent fallback refreshes

Update AMP Customizer admin menu link to set initial URL for amped post

556cacf

Restrict screen-reader-text to Customizer preview styles

a38d024

Also use AMP_QUERY_VAR instaed of hard-coded 'amp' in amp_admin_get_preview_permalink()

westonruter approved these changes Dec 10, 2017

View reviewed changes

westonruter mentioned this pull request Dec 10, 2017

Support for Pages #176

Closed

ThierryA approved these changes Dec 11, 2017

View reviewed changes

westonruter mentioned this pull request Dec 11, 2017

Add AMP support (experimental) xwp/wp-customize-posts#379

Open

westonruter added 7 commits December 11, 2017 13:40

Merge branch 'develop' of https://github.com/Automattic/amp-wp into f…

babf16c

…eature/796-amp-customizer

Remove deprecated and obsolete calls to amp_load_classes()

083c9b3

Let link in AMP toggle be regular link with href

64f14be

Add accessibility to AMP toggle; auto-show tooltip when navigating to…

219f481

… unavailable URL

Only show tooltip upon unavailable if AMP is enabled.

7308a9c

* Remove linebreak from tooltip for alignment with autosave restore notification; tweak copy. * Move functions from inside panel ready callback to component. * Improve styling in AMP when unloading page.

Show AMP unavailable tooltip when expanding AMP panel and not on AMP-…

79297bf

…compatible URL Improve styling of link in tooltip to improve color contrast

Disable AMP when switching away from mobile

b1fa356

westonruter merged commit d952ea8 into develop Dec 12, 2017

westonruter deleted the feature/796-amp-customizer branch December 12, 2017 06:44

westonruter added this to the v0.6 milestone Dec 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#796 Move AMP Customizer into main Customizer #819

#796 Move AMP Customizer into main Customizer #819

kaitnyl commented Dec 6, 2017 •

edited by westonruter

Loading

westonruter left a comment

westonruter Dec 6, 2017

westonruter Dec 6, 2017

westonruter Dec 6, 2017

westonruter Dec 6, 2017

westonruter Dec 6, 2017

kaitnyl Dec 7, 2017

westonruter Dec 6, 2017

westonruter Dec 6, 2017

westonruter Dec 6, 2017

westonruter Dec 6, 2017

westonruter Dec 6, 2017

kaitnyl commented Dec 7, 2017

westonruter commented Dec 7, 2017

westonruter Dec 7, 2017

westonruter Dec 7, 2017

westonruter Dec 7, 2017

westonruter Dec 7, 2017

kaitnyl commented Dec 8, 2017

westonruter commented Dec 9, 2017

ThierryA left a comment

ThierryA Dec 11, 2017 •

edited

Loading

westonruter Dec 12, 2017

ThierryA Dec 11, 2017

ThierryA Dec 11, 2017

westonruter Dec 11, 2017

westonruter commented Dec 11, 2017

ThierryA commented Dec 12, 2017

#796 Move AMP Customizer into main Customizer #819

#796 Move AMP Customizer into main Customizer #819

Conversation

kaitnyl commented Dec 6, 2017 • edited by westonruter Loading

westonruter left a 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

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

kaitnyl commented Dec 7, 2017

westonruter commented Dec 7, 2017

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

kaitnyl commented Dec 8, 2017

westonruter commented Dec 9, 2017

ThierryA left a comment

Choose a reason for hiding this comment

ThierryA Dec 11, 2017 • edited Loading

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

westonruter commented Dec 11, 2017

ThierryA commented Dec 12, 2017

kaitnyl commented Dec 6, 2017 •

edited by westonruter

Loading

ThierryA Dec 11, 2017 •

edited

Loading