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

Revamp notice appearing in AMP Enabled toggle, revamp how plugin determines whether AMP can be disabled #5010

Merged
merged 27 commits into from Jul 14, 2020

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jul 9, 2020

Summary

Fixes #1864.
Fixes #2724.
FIxes #4593.

This will require heavy rewriting of https://amp-wp.org/documentation/how-the-plugin-works/amp-plugin-serving-strategies/

AMP Unavailable Notice

  • When a post cannot be served as AMP, show the notice as informational rather than as a warning.
  • Start the notice with AMP Unavailable to give context for what the errors are.
  • When a link appears in a notice, have it open in a new window.
  • In the Classic editor, the notice continues to be displayed only when clicking the "Edit" link. In Gutenberg, the notice is displayed by default but inside of a details element which is expanded to show the notices.
  • If the user cannot manage_options then not AMP toggle is displayed and no notice is shown. Non-administrators will not see the toggle nor any notice if AMP is unavailable, sine they do not have the capability to take any action anyway.
Change Gutenberg Classic
Before image (After clicking the edit link) image
After (Collapsed) image (Looks same as before) image
After (Expanded) image image
After (Non-admin) image image

DB Upgrade

  • Wait until the end of init to perform the DB upgrade. This ensures that themes have declared theme support and post type support which we'll then persist in the DB going forward.

Theme Support

  • When add_theme_support('amp') is present, default to paired as being true as opposed to false. This only impacts the default theme_support value the first time the plugin is activated.
  • The templates_supported flag now only affects the default value for the options for all_templates_supported and supported_templates (more below).

Post Types

  • Eliminate the use of amp post type support to programmatically override the ability for the user to enable or disable a AMP for a given post type.
  • Migrate to storing options as associative arrays: store supported_post_types option as associative array mapping the post type to a boolean. Migrate previous representation of string array.
  • The first time the plugin is activated, when the amp-options option is empty: use the amp post type support to populate the default supported_post_types.

Templates

  • Eliminate the use of amp theme support to programmatically override the ability for the user to control whether all templates are supported and which specific templates are supported.
  • Eliminate the ability to use the amp_supportable_templates filter to programmatically override whether a template supported in AMP. If a filter tries to programmatically supply supported or immutable then _doing_it_wrong() is raised.
  • Migrate to storing options as associative arrays: store supported_templates option as an associative array mapping template ID to a boolean. Migrate previous representation of string array.
  • The first time the plugin is activated, when the amp-options option is empty: Use the templates_supported theme support arg to specify the default supported_templates option (make it false if the theme support arg is not 'all', and if it is an array use it to merge with the default supported_templates value.

Testing

To test, it's helpful to write out this JSON out to a file (e.g. options.json):

{"theme_support":"transitional","supported_post_types":["post"],"analytics":[],"all_templates_supported":false,"supported_templates":["is_singular"],"version":"1.5.5"}

And then load up the old plugin options via:

cat options.json | wp option set amp-options --format=json

It's also helpful to have these plugins to activate/deactivate to see how they impact the migration:

<?php
/**
 * Plugin Name: AMP Add Post Type Support for Pages
 */

add_action( 'init', function() {
	add_post_type_support( 'page', 'amp' );
} );
<?php
/**
 * Plugin Name: AMP Add Theme Support Templates Supported All
 */

add_action(
	'after_setup_theme',
	function () {
		add_theme_support( 'amp', [ 'templates_supported' => 'all' ] );
	}
);
<?php
/**
 * Plugin Name: AMP Add Theme Support Templates Supported Selectively
 */

add_action(
	'after_setup_theme',
	function () {
		add_theme_support(
			'amp',
			[
				'templates_supported' => [
					'is_search' => true,
					'is_404'    => false,
					'is_date'   => true,
				],
			]
		);
	}
);
<?php
/**
 * Plugin Name: AMP Skip Post All
 */

add_filter( 'amp_skip_post', '__return_true' );

Other

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.6 milestone Jul 9, 2020
@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 9, 2020
@westonruter westonruter mentioned this pull request Jul 9, 2020
3 tasks
…eme-support-paired-default

* 'develop' of github.com:ampproject/amp-wp: (94 commits)
  Improve formatting of active theme notice
  Open ecosystem page in a new window
  Remove dead code and update translator comment formatting
  Settings page CSS updates and test fixes
  Restore supported templates fieldset without obsolete wrapper
  Remove unused variable
  Add escaping to menu strings
  Make details prop a string instead of a node
  Escaping and minor updates to OptionsMenu.php
  Remove redundant isset check
  Rename reader-mode-override.js
  Comment formatting fix
  Change registration action for GoogleFonts class
  Restore ElementName
  Change ElementName to elementName and lint fixes
  Remove unnecessary default option provided via filter elsewhere
  Supported templates visibility updates
  Add todo indicating Plugin Suppression will be handled over REST in the future
  Increase width of status column in plugin suppression table
  Improve readability around mobile redirection visibility
  ...
@westonruter westonruter marked this pull request as ready for review July 10, 2020 22:21
@westonruter westonruter requested a review from pierlon July 10, 2020 22:22
@github-actions
Copy link
Contributor

github-actions bot commented Jul 10, 2020

Plugin builds for e07656a are ready 🛎️!

includes/amp-helper-functions.php Outdated Show resolved Hide resolved
includes/options/class-amp-options-manager.php Outdated Show resolved Hide resolved
* @return array $error_messages The error messages, as an array of strings.
*/
public function get_error_messages( $status, $errors ) {
public function get_error_messages( $errors ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think AMP_Post_Meta_Box::get_status_and_errors should be refactored into 2 separate methods? The returned status is now only being used in 1 instance (AMP_Post_Meta_Box::get_amp_enabled_rest_field), and the set of errors are used twice elsewhere (AMP_Post_Meta_Box::enqueue_block_assets and AMP_Post_Meta_Box::render_status).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's being used both in AMP_Post_Meta_Box::sanitize_status() and AMP_Post_Meta_Box::render_status(), in the latter case it appears to not be used only because the usage is in the include file at the end of the method.

Nevertheless, I'm not in love with get_status_and_errors.

src/Admin/OptionsMenu.php Outdated Show resolved Hide resolved
tests/php/test-amp-helper-functions.php Outdated Show resolved Hide resolved
tests/php/test-amp-helper-functions.php Outdated Show resolved Hide resolved
tests/php/test-amp.php Outdated Show resolved Hide resolved
tests/php/test-class-amp-options-manager.php Show resolved Hide resolved
tests/php/test-class-amp-options-manager.php Outdated Show resolved Hide resolved
* Hide if user can't manage_options anyway.
* Open links to setting screen in new window.
* Add "AMP Unavailable" heading to notice.
* Use <details> to contain error messages in notice in block editor.
* Use notice as opposed to warning.
westonruter and others added 2 commits July 11, 2020 00:49
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Comment on lines -347 to -353
if ( in_array( 'status_immutable', $errors, true ) ) {
if ( self::ENABLED_STATUS === $status ) {
$error_messages[] = __( 'Your site does not allow AMP to be disabled.', 'amp' );
} else {
$error_messages[] = __( 'Your site does not allow AMP to be enabled.', 'amp' );
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Byebye!

…eme-support-paired-default

* 'develop' of github.com:ampproject/amp-wp:
  Update dependency dealerdirect/phpcodesniffer-composer-installer to v0.7.0
  Harden ReaderThemesTest when custom theme directories present
  Update dependency sirbrillig/phpcs-variable-analysis to v2.8.3
  Skip test in WP 4.9
  Fix grammar typo
  Remove escaping of title in legacy post templates (#5026)
  Skip test when Twenty Twenty is not installed
  Prevent Cover Template section in AMP Customizer from appearing when Twenty Twenty used as Reader theme
  Add is_theme_overridden method on ReaderThemeLoader service
  Improve detection for whether or not ReaderThemeSwitcher is enabled
  Make ReaderThemeLoaer unconditional and add method to obtain originally-active theme
  Prevent sanitization of common style elements in the Customizer preview
  Fix lint issue in test
  Fix lint issue
  Add test
  Prevent sanitization of styles in some Core themes requried for some Customizer features to work correctly
@westonruter westonruter changed the title Eliminate reliance on theme support flags and filters to control plugin behavior Revamp notice appearing in AMP Enabled toggle, revamp how plugin determines whether AMP can be disabled Jul 12, 2020
@westonruter westonruter requested a review from pierlon July 13, 2020 18:48
…eme-support-paired-default

* 'develop' of github.com:ampproject/amp-wp: (36 commits)
  Fix typo in comment
  Update dependency @wordpress/plugins to v2.20.1
  Improve copy for panel description and control notice
  Handle case where user resets devtools option back to original
  Remove extraneous em unit from line-height
  Add link to customize non-AMP version
  Update AMP panel description in Customizer
  Update dependency eslint-plugin-react-hooks to v4.0.8
  Add AMP panel description to the AMP Customizer root
  Update dependency @wordpress/scripts to v12.1.1
  Update dependency @wordpress/components to v10.0.1
  Update dependency eslint-plugin-react to v7.20.3
  Update dependency eslint-plugin-jest to v23.18.0
  Update dependency eslint-plugin-jsdoc to v29.2.0
  Add notification to the menus panel explaining menus are shared
  Use wp.customize alias
  Simplify logic for removing Cover Template section
  Remove the Homepage Settings section in AMP Customizer for a Reader theme
  Add notice to all Customizer controls for option settings
  Re-organize functions in amp-customize-controls
  ...
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality works as intended, only a minor visual fix I'd like to suggest.

assets/css/src/amp-block-editor.css Show resolved Hide resolved
assets/css/src/amp-block-editor.css Show resolved Hide resolved
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
@westonruter
Copy link
Member Author

Just noticed that WordPress 5.5 removes the blue background for the notices. The margins are also a bit off, but this is out of scope of this PR. For example, the Featured Image notice:

image

@westonruter westonruter requested a review from pierlon July 14, 2020 04:28
@westonruter westonruter merged commit 9f7b74c into develop Jul 14, 2020
@westonruter westonruter deleted the update/theme-support-paired-default branch July 14, 2020 05:20
@westonruter
Copy link
Member Author

Two follow-up items I've addressed in #5042:

  • AMP_Core_Theme_Sanitizer::extend_theme_support() would fail to add amp theme support if a core theme doesn't have any special theme support args. This resulted in Twenty Sixteen getting Transitional mode by default, but Twenty Twenty did not.
  • When a child theme of a core theme is active, we should not default to Transitional or Standard because all bets are off as to whether it actually supports AMP. Fixed in f8c8390. We'll only be able to change that once we have the theme scan feature (Wizard Step: Theme compatibility scanning #4795).

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@jwold jwold added the WS:UX Work stream for UX/Front-end label Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA WS:UX Work stream for UX/Front-end
Projects
None yet
4 participants