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

#796 Move AMP Customizer into main Customizer #819

Merged
merged 27 commits into from Dec 12, 2017

Conversation

Projects
None yet
3 participants
@kaitnyl
Copy link
Contributor

kaitnyl commented Dec 6, 2017

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.

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

@kaitnyl kaitnyl self-assigned this Dec 6, 2017

@kaitnyl kaitnyl requested review from westonruter and ThierryA Dec 6, 2017

@westonruter
Copy link
Member

westonruter left a comment

@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”

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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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.

var ampEnabled = false,
headContent = $( api.previewer.preview.iframe ).contents().find( 'head' ).html();

if ( headContent.search( '<meta name="generator" content="AMP Plugin' ) >= 0 ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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.

// 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 comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

This needs be translated.

'</label>' );

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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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.

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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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 comment has been minimized.

Copy link
@kaitnyl

kaitnyl Dec 7, 2017

Author Contributor

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


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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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();
} );
} )( api.previewer.previewUrl.validate );

// Adding checkbox toggle before device selection.
$( '.devices-wrapper' ).before( '<label class="amp-toggle">' +

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

This can make use of a wp.template.

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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

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

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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

The ampToggle here could be defined locally as:

var ampToggle = wp.customize.state( 'ampEnabled' ).get() && wp.customize.state( 'ampAvailable' ).get();
*
* @return {void}
*/
function toggleAmpView() {

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 6, 2017

Member

This might be better named updatePreviewUrl

westonruter and others added some commits Dec 6, 2017

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

* Use query var in permalinks containing query vars, instead of endpoint slug.
* Remove needless and unslightly 1 value.
* Add tests.
@kaitnyl

This comment has been minimized.

Copy link
Contributor Author

kaitnyl commented Dec 7, 2017

@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?

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 7, 2017

@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.

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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 7, 2017

Member

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 ] ) ) {
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, '' );

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 7, 2017

Member

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

@@ -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(

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 7, 2017

Member

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).


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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 7, 2017

Member

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.

@kaitnyl

This comment has been minimized.

Copy link
Contributor Author

kaitnyl commented Dec 8, 2017

@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).

kaitnyl and others added some commits Dec 8, 2017

Fix live preview of color changes
* 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.
Add selective refresh of header-bar to customizer preview
Fix obsolete method references and improve placement of add_action() calls
Improve behavior of AMP toggle when loading initially-AMPed URL in pr…
…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

This comment has been minimized.

Copy link
Member

westonruter commented Dec 9, 2017

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

Restrict screen-reader-text to Customizer preview styles
Also use AMP_QUERY_VAR instaed of hard-coded 'amp' in amp_admin_get_preview_permalink()

@westonruter westonruter referenced this pull request Dec 10, 2017

Closed

Support for Pages #176

@ThierryA
Copy link
Collaborator

ThierryA left a comment

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.

$tooltip = $( '.tooltip', $( this ) ),
tooltipTimer = 5000;

if ( $input.prop( 'disabled' ) ) {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 11, 2017

Collaborator

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

This comment has been minimized.

Copy link
@westonruter

westonruter Dec 12, 2017

Member

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

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 11, 2017

Collaborator

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

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

This comment has been minimized.

Copy link
@ThierryA

ThierryA Dec 11, 2017

Collaborator

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

This comment has been minimized.

Copy link
@westonruter
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Dec 11, 2017

@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?

westonruter added some commits Dec 11, 2017

Only show tooltip upon unavailable if AMP is enabled.
* 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-…
…compatible URL

Improve styling of link in tooltip to improve color contrast

@westonruter westonruter merged commit d952ea8 into develop Dec 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the feature/796-amp-customizer branch Dec 12, 2017

@ThierryA

This comment has been minimized.

Copy link
Collaborator

ThierryA commented Dec 12, 2017

Great improvements @westonruter ❤️

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.