-
Notifications
You must be signed in to change notification settings - Fork 381
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
Add Gutenberg 'Enable AMP' toggle #1275
Merged
Merged
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
5fb8d66
Begin Gutenberg 'Enabled AMP' toggle.
kienstra 9f57e67
Refactor JS file to use a module patter.
kienstra a06e32d
Abstract logic to get enabled status into helper method.
kienstra c328564
Add assertions for inline script.
kienstra acc6467
Improve documentation in amp-block-editor-toggle.js.
kienstra c05e469
Address Travis error by removing blank line.
kienstra 5ed6837
Take first step to enable compiling amp-block-editor-togge.js
kienstra 69d6c77
Begin to convert AMP block editor toggle to a module.
kienstra 46723ad
Merge branch 'develop' into add/editor-toggle
kienstra f59170c
Improve documentation, simplify conditional.
kienstra 25156f4
Localize values for amp-block-editor-toggle.js
kienstra d5378c5
Move error messages into function, localize them into script
kienstra 0735ed0
Output Notices with links in the block editor.
kienstra f8c3c09
Improve documentation for Notices
kienstra 8b5d749
Use wildcards in .eslintignore and .jshintignore
kienstra c7bc94c
Only call gutenberg_get_jed_locale_data() if it exists.
kienstra 9f0e248
Simply pass value to function without storing in variable
kienstra b76886a
Simplify error messages to use RawHTML.
kienstra 230d87a
Fix test_get_error_messages() to apply latest change.
kienstra c4da67b
Fix error raised when post resource lacks meta
westonruter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
**/*.min.js | ||
**/node_modules/** | ||
**/vendor/** | ||
**/assets/js/amp-blocks-compiled.js | ||
**/assets/js/*-compiled.js |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
**/*.min.js | ||
**/node_modules/** | ||
**/vendor/** | ||
**/assets/js/amp-blocks-compiled.js | ||
**/assets/js/*-compiled.js |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
const { __ } = wp.i18n; | ||
const { FormToggle, Notice } = wp.components; | ||
const { Fragment } = wp.element; | ||
const { withSelect, withDispatch } = wp.data; | ||
const { PluginPostStatusInfo } = wp.editPost; | ||
const { compose, withInstanceId } = wp.compose; | ||
|
||
/** | ||
* Exported via wp_localize_script(). | ||
*/ | ||
const { possibleStati, defaultStatus, errorMessages } = window.wpAmpEditor; | ||
|
||
/** | ||
* Adds an 'Enable AMP' toggle to the block editor 'Status & Visibility' section. | ||
* | ||
* If there are error(s) that block AMP from being enabled or disabled, | ||
* this only displays a Notice with the error(s), not a toggle. | ||
* Error(s) are imported as errorMessages via wp_localize_script(). | ||
* | ||
* @return {Object} AMPToggle component. | ||
*/ | ||
function AMPToggle( { enabledStatus, onAmpChange } ) { | ||
return ( | ||
<Fragment> | ||
<PluginPostStatusInfo> | ||
{ ! errorMessages.length && __( 'Enable AMP', 'amp' ) } | ||
{ | ||
! errorMessages.length && | ||
( | ||
<FormToggle | ||
checked={ 'enabled' === enabledStatus } | ||
onChange={ () => onAmpChange( enabledStatus ) } | ||
id={ 'amp-enabled' } | ||
/> | ||
) | ||
} | ||
{ | ||
!! errorMessages.length && | ||
( | ||
<Notice | ||
status={ 'warning' } | ||
isDismissible={ false } | ||
> | ||
{ | ||
errorMessages.map( function( message ) { | ||
let minSplitLength = 2; | ||
|
||
if ( 'string' === typeof message ) { | ||
// The message is only a string, so return it. | ||
return message; | ||
} | ||
if ( message[ 0 ].split( '%s' ).length > minSplitLength ) { | ||
/** | ||
* The message is an array with the text in the 0 index, and the href in the 1 index. | ||
* And the text should have two %s as placeholders for <a>, like 'AMP cannot be enabled because this %spost type does not support it%s.'. | ||
* So split it along %s, to construct the message with the <a>, like: | ||
* 'AMP cannot be enabled because this <a href="foo">post type does not support it</a>.'. | ||
*/ | ||
let splitMessage = message[ 0 ].split( '%s' ); | ||
return ( <p>{ splitMessage[ 0 ] }<a href={ message[ 1 ] }>{ splitMessage[ 1 ] }</a>{ splitMessage[ 2 ] }</p> ); | ||
} | ||
} ) | ||
} | ||
</Notice> | ||
) | ||
} | ||
</PluginPostStatusInfo> | ||
</Fragment> | ||
); | ||
} | ||
|
||
/** | ||
* The AMP Toggle component, composed with the enabledStatus and a callback for when it's changed. | ||
* | ||
* @return {Object} The composed AMP toggle. | ||
*/ | ||
function ComposedAMPToggle() { | ||
return compose( [ | ||
withSelect( ( select ) => { | ||
/** | ||
* Gets the AMP enabled status. | ||
* | ||
* Uses select from the enclosing function to get the meta value. | ||
* If it doesn't exist, it uses the default value. | ||
* This applies especially for a new post, where there probably won't be a meta value yet. | ||
* | ||
* @return {string} Enabled status, either 'enabled' or 'disabled'. | ||
*/ | ||
let getEnabledStatus = function() { | ||
let metaSetatus = select( 'core/editor' ).getEditedPostAttribute( 'meta' ).amp_status; | ||
if ( possibleStati.includes( metaSetatus ) ) { | ||
return metaSetatus; | ||
} | ||
return defaultStatus; | ||
}; | ||
|
||
return { enabledStatus: getEnabledStatus() }; | ||
} ), | ||
withDispatch( ( dispatch ) => ( { | ||
onAmpChange: function( enabledStatus ) { | ||
let newStatus = 'enabled' === enabledStatus ? 'disabled' : 'enabled'; | ||
dispatch( 'core/editor' ).editPost( { meta: { amp_status: newStatus } } ); | ||
} | ||
} ) ), | ||
withInstanceId | ||
] )( AMPToggle ); | ||
} | ||
|
||
export default wp.plugins.registerPlugin( 'amp', { | ||
icon: 'hidden', | ||
render: ComposedAMPToggle() | ||
} ); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,36 +39,7 @@ | |
</fieldset> | ||
<?php else : ?> | ||
<div class="inline notice notice-warning notice-alt"> | ||
<p> | ||
<?php | ||
$error_messages = array(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section is moved to get_raw_error_messages(). |
||
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' ); | ||
} | ||
} | ||
if ( in_array( 'template_unsupported', $errors, true ) || in_array( 'no_matching_template', $errors, true ) ) { | ||
/* translators: %s is URL to AMP settings screen */ | ||
$error_messages[] = wp_kses_post( sprintf( __( 'There are no <a href="%s">supported templates</a> to display this in AMP.', 'amp' ), esc_url( admin_url( 'admin.php?page=' . AMP_Options_Manager::OPTION_NAME ) ) ) ); | ||
} | ||
if ( in_array( 'password-protected', $errors, true ) ) { | ||
$error_messages[] = __( 'AMP cannot be enabled on password protected posts.', 'amp' ); | ||
} | ||
if ( in_array( 'post-type-support', $errors, true ) ) { | ||
/* translators: %s is URL to AMP settings screen */ | ||
$error_messages[] = wp_kses_post( sprintf( __( 'AMP cannot be enabled because this <a href="%s">post type does not support it</a>.', 'amp' ), esc_url( admin_url( 'admin.php?page=' . AMP_Options_Manager::OPTION_NAME ) ) ) ); | ||
} | ||
if ( in_array( 'skip-post', $errors, true ) ) { | ||
$error_messages[] = __( 'A plugin or theme has disabled AMP support.', 'amp' ); | ||
} | ||
if ( count( array_diff( $errors, array( 'status_immutable', 'page-on-front', 'page-for-posts', 'password-protected', 'post-type-support', 'skip-post', 'template_unsupported', 'no_matching_template' ) ) ) > 0 ) { | ||
$error_messages[] = __( 'Unavailable for an unknown reason.', 'amp' ); | ||
} | ||
echo implode( ' ', $error_messages ); // WPCS: xss ok. | ||
?> | ||
</p> | ||
<p><?php echo implode( ' ', $error_messages ); // WPCS: xss ok. ?></p> | ||
</div> | ||
<?php endif; ?> | ||
<div class="amp-status-actions"> | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
There is duplication between PHP here and the JS in
PluginPostStatusInfo
. I appreciate the concern to notdangerouslySetInnerHTML
but since the error messages are coming from a trusted source, I think it would be better to do so in this case. Even in Gutenberg, for example, there is aRawHTML
component for this purpose. This component is publicly available already in Gutenberg viawp.element.RawHTML
.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.
Thanks, I'm working on this now.
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.
These commits simplify the error messages to use
RawHTML
like you suggested.