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.

Sign up for GitHub

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

Reuse validation error titles from validated URL screen in block warning notice #4401

Merged
merged 8 commits into from
Mar 18, 2020
Merged

Reuse validation error titles from validated URL screen in block warning notice #4401

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dcd9794
Ensure font-family of block error list items is system font
westonruter Mar 17, 2020
1e8b1f7
Send validation error titles back in REST API to reuse in block edito…
westonruter Mar 17, 2020
4a59e23
Adapt test to new props
pierlon Mar 17, 2020
677a9d6
Update test snapshot
pierlon Mar 17, 2020
efbad91
Add whitespace to make test pass
pierlon Mar 18, 2020
7041680
Improve reliability of AMP_Facebook_Embed_Test::test__raw_embed_sanit…
westonruter Mar 18, 2020
aa9593d
Merge branch 'develop' of github.com:ampproject/amp-wp into add/valid…
westonruter Mar 18, 2020
3c2ae84
Normalize additional blockquotes after merge conflict
westonruter Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion assets/src/block-validation/components/higher-order/with-validation-error-notice/edit.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
.amp-block-validation-errors {
.amp-block-validation-errors,
.amp-block-validation-errors * {
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;
font-size: 13px;
line-height: 1.5;
Expand Down
34 changes: 8 additions & 26 deletions assets/src/block-validation/components/validation-error-message/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,42 +7,25 @@ import { ReactElement } from 'react';
/**
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { __ } from '@wordpress/i18n';

/**
* Get message for validation error.
*
* @param {Object} props Component props.
* @param {Object} props Component props.
* @param {?string} props.title Title for error (with HTML) as provided by \AMP_Validation_Error_Taxonomy::get_error_title_from_code().
* @param {?string} props.code Error code.
* @param {?string} props.node_name Node name.
* @param {?string} props.parent_name Parent node name.
* @param {?string|ReactElement} props.message Error message.
*
* @return {ReactElement} Validation error message.
*/
const ValidationErrorMessage = ( { message, code, node_name: nodeName, parent_name: parentName } ) => {
const ValidationErrorMessage = ( { title, message, code } ) => {
if ( message ) {
return message;
return message; // @todo It doesn't appear this is ever set?
Copy link
Contributor

Choose a reason for hiding this comment

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

The REST response result doesn't include a message key, so I don't think it's set. I couldn't find any reference to it either in any other React component... maybe @swissspidy could shine a light on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems it's only be used in its test:

amp-wp/assets/src/block-validation/components/validation-error-message/test/index.js

Line 13 in 4a59e23

const errorMessage = render( <ValidationErrorMessage message="Hello World" /> );

}

if ( 'DISALLOWED_TAG' === code && nodeName ) { // @todo Needs to be fleshed out.
return (
<>
{ __( 'Invalid element: ', 'amp' ) }
<code>
{ nodeName }
</code>
</>
);
} else if ( 'DISALLOWED_ATTR' === code && nodeName ) { // @todo Needs to be fleshed out.
return (
<>
{ __( 'Invalid attribute: ', 'amp' ) }
<code>
{ parentName ? sprintf( '%s[%s]', parentName, nodeName ) : nodeName }
</code>
</>
);
if ( title ) {
return <span dangerouslySetInnerHTML={ { __html: title } } />
}

return (
Expand All @@ -57,9 +40,8 @@ const ValidationErrorMessage = ( { message, code, node_name: nodeName, parent_na

ValidationErrorMessage.propTypes = {
message: PropTypes.string,
title: PropTypes.string,
code: PropTypes.string,
node_name: PropTypes.string,
parent_name: PropTypes.string,
};

export default ValidationErrorMessage;
3 changes: 2 additions & 1 deletion assets/src/block-validation/helpers/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,15 @@ export const updateValidationErrors = () => {
/**
* @param {Object} result Validation error result.
* @param {Object} result.error Error object.
* @param {string} result.title Error title.
* @param {boolean} result.forced Whether sanitization was forced.
* @param {boolean} result.sanitized Whether the error has been sanitized or not.
* @param {number} result.status Validation error status.
* @param {number} result.term_status Error status.
*/
const validationErrors = ampValidity.results.filter( ( result ) => {
return result.term_status !== VALIDATION_ERROR_ACK_ACCEPTED_STATUS; // If not accepted by the user.
} ).map( ( { error, status } ) => ( { ...error, status } ) ); // Merge status into error since needed in maybeDisplayNotice.
} ).map( ( { error, status, title } ) => ( { ...error, status, title } ) ); // Merge status into error since needed in maybeDisplayNotice.

if ( isEqual( validationErrors, previousValidationErrors ) ) {
return;
Expand Down
1 change: 1 addition & 0 deletions includes/validation/class-amp-validation-manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,7 @@ public static function get_amp_validity_rest_field( $post_data, $field_name, $re
foreach ( AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( $validation_status_post ) as $result ) {
$field['results'][] = [
'sanitized' => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_ACCEPTED_STATUS === $result['status'],
'title' => AMP_Validation_Error_Taxonomy::get_error_title_from_code( $result['data'] ),
'error' => $result['data'],
'status' => $result['status'],
'term_status' => $result['term_status'],
Expand Down
1 change: 1 addition & 0 deletions tests/php/validation/test-class-amp-validation-manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,7 @@ public function test_get_amp_validity_rest_field() {
static function ( $error ) {
return [
'sanitized' => false,
'title' => 'Unknown error (test)',
'error' => $error,
'status' => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS,
'term_status' => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS,
Expand Down