-
Notifications
You must be signed in to change notification settings - Fork 382
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
Conversation
…lter to force templates to be supported
…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 ...
…for default value
Plugin builds for e07656a are ready 🛎️!
|
* @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 ) { |
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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
.
* 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.
…d supported templates
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
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' ); | ||
} | ||
} |
There was a problem hiding this comment.
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
…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 ...
There was a problem hiding this 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.
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Two follow-up items I've addressed in #5042:
|
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
details
element which is expanded to show the notices.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.DB Upgrade
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
add_theme_support('amp')
is present, default topaired
as beingtrue
as opposed tofalse
. This only impacts the defaulttheme_support
value the first time the plugin is activated.templates_supported
flag now only affects the default value for the options forall_templates_supported
andsupported_templates
(more below).Post Types
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: storesupported_post_types
option as associative array mapping the post type to a boolean. Migrate previous representation of string array.amp-options
option is empty: use theamp
post type support to populate the defaultsupported_post_types
.Templates
amp
theme support to programmatically override the ability for the user to control whether all templates are supported and which specific templates are supported.amp_supportable_templates
filter to programmatically override whether a template supported in AMP. If a filter tries to programmatically supplysupported
orimmutable
then_doing_it_wrong()
is raised.Migrate to storing options as associative arrays: storesupported_templates
option as an associative array mapping template ID to a boolean. Migrate previous representation of string array.amp-options
option is empty: Use thetemplates_supported
theme support arg to specify the defaultsupported_templates
option (make itfalse
if the theme support arg is not'all'
, and if it is an array use it to merge with the defaultsupported_templates
value.Testing
To test, it's helpful to write out this JSON out to a file (e.g.
options.json
):And then load up the old plugin options via:
It's also helpful to have these plugins to activate/deactivate to see how they impact the migration:
Other
Checklist