Show details with each block's validation errors; improve styling

ampproject · Apr 11, 2018 · 21e60f0 · 21e60f0
1 parent 4d7dc19
commit 21e60f0
Show file tree

Hide file tree

Showing 4 changed files with 91 additions and 26 deletions.
diff --git a/assets/css/amp-post-meta-box.css b/assets/css/amp-post-meta-box.css
@@ -64,3 +64,16 @@
 		line-height: 280%;
 	}
 }
+
+.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;
+}
+.amp-block-validation-errors .amp-block-validation-errors__summary {
+	margin: 0.5em 0;
+	padding: 2px;
+}
+.amp-block-validation-errors .amp-block-validation-errors__list {
+	padding-left: 2.5em;
+}
diff --git a/assets/js/amp-block-validation.js b/assets/js/amp-block-validation.js
@@ -18,9 +18,8 @@ var ampBlockValidation = ( function() {
 		 * @param {Object}
 		 */
 		data: {
-			i18n: {
-				invalidAmpContentNotice: ''
-			}
+			i18n: {},
+			restValidationErrorsField: ''
 		},
 
 		/**
@@ -39,6 +38,8 @@ var ampBlockValidation = ( function() {
 		boot: function boot( data ) {
 			module.data = data;
 
+			wp.i18n.setLocaleData( module.data.i18n, 'amp' );
+
 			wp.hooks.addFilter(
 				'blocks.BlockEdit',
 				'amp/add-notice',
@@ -126,8 +127,8 @@ var ampBlockValidation = ( function() {
 				wp.i18n._n(
 					'There is %s issue from AMP validation.',
 					'There are %s issues from AMP validation.',
-					validationErrors.length
-					// @todo Domain.
+					validationErrors.length,
+					'amp'
 				),
 				validationErrors.length
 			);
@@ -140,20 +141,20 @@ var ampBlockValidation = ( function() {
 				if ( blockErrorCount > 0 ) {
 					noticeMessage += ' ' + wp.i18n.sprintf(
 						wp.i18n._n(
-							'And %s is directly due to the content here.',
-							'And %s are directly due to the content here.',
-							blockErrorCount
-							// @todo Domain.
+							'And %s is directly due to content here.',
+							'And %s are directly due to content here.',
+							blockErrorCount,
+							'amp'
 						),
 						blockErrorCount
 					);
 				} else {
 					noticeMessage += ' ' + wp.i18n.sprintf(
 						wp.i18n._n(
-							'But it is not directly due to the content here.',
-							'But none are directly due to the content here.',
-							validationErrors.length
-							// @todo Domain.
+							'But it is not directly due to content here.',
+							'But none are directly due to content here.',
+							validationErrors.length,
+							'amp'
 						),
 						validationErrors.length
 					);
@@ -163,13 +164,15 @@ var ampBlockValidation = ( function() {
 				wp.data.dispatch( module.storeName ).updateBlocksValidationErrors( {} );
 
 				noticeMessage += ' ' + wp.i18n._n(
-					'It may not be due to the content here.',
-					'Some may be due to the content here.',
-					validationErrors.length
-					// @todo Domain.
+					'It may not be due to content here.',
+					'Some may be due to content here.',
+					validationErrors.length,
+					'amp'
 				);
 			}
 
+			noticeMessage += ' ' + wp.i18n.__( 'Invalid code is stripped when displaying AMP.', 'amp' );
+
 			module.validationWarningNoticeId = wp.data.dispatch( 'core/editor' ).createWarningNotice( noticeMessage ).notice.id;
 		},
 
@@ -260,6 +263,36 @@ var ampBlockValidation = ( function() {
 			};
 		},
 
+		/**
+		 * Get message for validation error.
+		 *
+		 * @param {Object} validationError - Validation error.
+		 * @param {string} validationError.code - Validation error code.
+		 * @param {string} [validationError.node_name] - Node name.
+		 * @param {string} [validationError.message] - Validation error message.
+		 * @return {wp.element.Component[]|string[]} Validation error message.
+		 */
+		getValidationErrorMessage: function getValidationErrorMessage( validationError ) {
+			if ( validationError.message ) {
+				return validationError.message;
+			}
+			if ( 'invalid_element' === validationError.code && validationError.node_name ) {
+				return [
+					wp.i18n.__( 'Invalid element: ' ),
+					wp.element.createElement( 'code', { key: 'name' }, validationError.node_name )
+				];
+			} else if ( 'invalid_attribute' === validationError.code && validationError.node_name ) {
+				return [
+					wp.i18n.__( 'Invalid attribute: ' ),
+					wp.element.createElement( 'code', { key: 'name' }, validationError.parent_name ? wp.i18n.sprintf( '%s[%s]', validationError.parent_name, validationError.node_name ) : validationError.node_name )
+				];
+			}
+			return [
+				wp.i18n.__( 'Error code: ', 'amp' ),
+				wp.element.createElement( 'code', { key: 'name' }, validationError.code || wp.i18n.__( 'unknown' ) )
+			];
+		},
+
 		/**
 		 * Wraps the edit() method of a block, and conditionally adds a Notice.
 		 *
@@ -268,7 +301,8 @@ var ampBlockValidation = ( function() {
 		 */
 		conditionallyAddNotice: function conditionallyAddNotice( BlockEdit ) {
 			function AmpNoticeBlockEdit( props ) {
-				var edit = wp.element.createElement(
+				var edit, details;
+				edit = wp.element.createElement(
 					BlockEdit,
 					_.extend( {}, props, { key: 'amp-original-edit' } )
 				);
@@ -277,17 +311,34 @@ var ampBlockValidation = ( function() {
 					return edit;
 				}
 
-				return [
+				details = wp.element.createElement( 'details', { className: 'amp-block-validation-errors' }, [
+					wp.element.createElement( 'summary', { key: 'summary', className: 'amp-block-validation-errors__summary' }, wp.i18n.sprintf(
+						wp.i18n._n(
+							'There is %s issue from AMP validation.',
+							'There are %s issues from AMP validation.',
+							props.ampBlockValidationErrors.length,
+							'amp'
+						),
+						props.ampBlockValidationErrors.length
+					) ),
+					wp.element.createElement(
+						'ul',
+						{ key: 'list', className: 'amp-block-validation-errors__list' },
+						_.map( props.ampBlockValidationErrors, function( error, key ) {
+							return wp.element.createElement( 'li', { key: key }, module.getValidationErrorMessage( error ) );
+						} )
+					)
+				] );
 
-					// @todo Add PanelBody with validation error details.
+				return [
 					wp.element.createElement(
 						wp.components.Notice,
 						{
 							status: 'warning',
 							isDismissible: false,
 							key: 'amp-validation-notice'
 						},
-						module.data.i18n.invalidAmpContentNotice + ' ' + _.pluck( props.ampBlockValidationErrors, 'code' ).join( ', ' )
+						details
 					),
 					edit
 				];

diff --git a/includes/utils/class-amp-validation-utils.php b/includes/utils/class-amp-validation-utils.php
@@ -1933,11 +1933,8 @@ public static function enqueue_block_validation() {
 			true
 		);
 
-		// @todo Add moreDetails and summary.
 		$data = wp_json_encode( array(
-			'i18n'                      => array(
-				'invalidAmpContentNotice' => __( 'This block has invalid AMP content:', 'amp' ),
-			),
+			'i18n'                      => gutenberg_get_jed_locale_data( 'amp' ), // @todo POT file.
 			'restValidationErrorsField' => self::REST_FIELD_NAME,
 		) );
 		wp_add_inline_script( $slug, sprintf( 'ampBlockValidation.boot( %s );', $data ) );

diff --git a/tests/test-class-amp-validation-utils.php b/tests/test-class-amp-validation-utils.php
@@ -1432,6 +1432,10 @@ public function test_get_recheck_link() {
 	 * @covers AMP_Validation_Utils::enqueue_block_validation()
 	 */
 	public function test_enqueue_block_validation() {
+		if ( ! function_exists( 'gutenberg_get_jed_locale_data' ) ) {
+			$this->markTestSkipped( 'Gutenberg not available.' );
+		}
+
 		global $post;
 		$post = $this->factory()->post->create_and_get(); // WPCS: global override ok.
 		$slug = 'amp-block-validation';
@@ -1446,7 +1450,7 @@ public function test_enqueue_block_validation() {
 		$this->assertTrue( in_array( $slug, wp_scripts()->queue, true ) );
 		$this->assertContains( 'ampBlockValidation.boot', $inline_script );
 		$this->assertContains( AMP_Validation_Utils::REST_FIELD_NAME, $inline_script );
-		$this->assertContains( 'This block has invalid AMP content', $inline_script );
+		$this->assertContains( '"domain":"amp"', $inline_script );
 	}
 
 	/**