Skip to content
This repository was archived by the owner on Oct 4, 2022. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6d4b06e
Minor CS fixes
johannadevos Mar 19, 2021
abf4526
WIP: Add missing recommended blocks to the validation.issues in Inner…
johannadevos Mar 19, 2021
3aea2f5
Add the BlockType enum to BlockValidationResult and, for the time bei…
johannadevos Mar 19, 2021
99477c2
Rename the variables in findMissingBlocks to make the function inclus…
johannadevos Mar 19, 2021
7031766
Pass the block type to the findMissingBlocks() function
johannadevos Mar 19, 2021
8cfa903
Convert the numeric enum to a string enum
johannadevos Mar 19, 2021
84014f8
Improve CS in imports
johannadevos Mar 23, 2021
96af683
Improve naming in findMissingBlocks function
johannadevos Mar 23, 2021
b2edab2
Undo changes made to the RecommendedBlock type
johannadevos Mar 23, 2021
47ceefc
Fix TS error where recommended blocks couldn't be passed to the findM…
johannadevos Mar 23, 2021
40a0c40
Improve function documentation
johannadevos Mar 23, 2021
fa7fa29
Convert the enum values to lower case
johannadevos Mar 23, 2021
abc47c0
Show an orange bullet in the analysis if the missing block is recomme…
johannadevos Mar 23, 2021
c562969
Make sure the analysis conclusion is still shown in case there are re…
johannadevos Mar 23, 2021
b8a4546
Improve copy for the sidebar analysis
johannadevos Mar 23, 2021
bc81d23
Fix CS in imports
johannadevos Mar 23, 2021
2a36ceb
Fix more CS in imports
johannadevos Mar 23, 2021
a99a09d
Improve documentation
johannadevos Mar 25, 2021
c3572ed
Fix eslint errors about the maximum length of a line
johannadevos Mar 25, 2021
53fa3a3
Add the new BlockType parameter for the BlockValidationResult to exis…
johannadevos Mar 25, 2021
248cd24
Add tests for recommended blocks
johannadevos Mar 25, 2021
4f4e252
Test the scenario where the validation results cannot be retrieved
johannadevos Mar 25, 2021
acea646
Merge branch 'develop' of https://github.com/Yoast/javascript into P2…
johannadevos Mar 26, 2021
cbd2bba
Rename requiredIssues to requiredBlockIssues
johannadevos Mar 26, 2021
5a558fe
Rename BlockType to BlockPresence
johannadevos Mar 26, 2021
40260b6
Rename 'status' property in analysisIssue type to 'presence'
johannadevos Mar 26, 2021
dd6d261
Make "One" the default quantity for all the blocks; there is no longe…
johannadevos Mar 26, 2021
0b0b5e2
Rename more occurrences of BlockType to BlockPresence
johannadevos Mar 26, 2021
61804b6
Apply the `findRedundantBlocks` validation to the recommended blocks …
johannadevos Mar 26, 2021
7281eab
Sort imports alphabetically
johannadevos Mar 26, 2021
5e2faf6
Add the blockPresence parameter to the `innerBlocksValid` test
johannadevos Mar 29, 2021
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 packages/schema-blocks/src/core/Definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { isArray, mergeWith } from "lodash";
import Instruction from "./Instruction";
import Leaf from "./Leaf";
import logger from "../functions/logger";
import { BlockPresence } from "./validation/BlockValidationResult";

export type DefinitionClass<T extends Definition> = {
new( separator: string, template?: string, instructions?: Record<string, Instruction>, tree?: Leaf ): T;
Expand Down Expand Up @@ -80,7 +81,7 @@ export default abstract class Definition {
return null;
}

const validation = new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Unknown );
const validation = new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Unknown, BlockPresence.Unknown );

logger.startGroup( `Validation results: ${ blockInstance.name }` );

Expand Down
3 changes: 2 additions & 1 deletion packages/schema-blocks/src/core/Instruction.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { BlockInstance } from "@wordpress/blocks";
import logger from "../functions/logger";
import { BlockValidationResult, BlockValidation } from "./validation";
import { BlockPresence } from "./validation/BlockValidationResult";
export type InstructionPrimitive = string | number | boolean;
export type InstructionValue = InstructionPrimitive | InstructionObject | InstructionArray;
export type InstructionArray = readonly InstructionValue[];
Expand Down Expand Up @@ -60,7 +61,7 @@ export default abstract class Instruction {
* @returns {BlockValidationResult} The validation result.
*/
validate( blockInstance: BlockInstance ): BlockValidationResult {
return new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Unknown );
return new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Unknown, BlockPresence.Unknown );
}

/**
Expand Down
13 changes: 8 additions & 5 deletions packages/schema-blocks/src/core/blocks/BlockInstruction.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import BlockLeaf from "./BlockLeaf";
import { RenderSaveProps, RenderEditProps } from "./BlockDefinition";
import { RenderEditProps, RenderSaveProps } from "./BlockDefinition";
import { ReactElement } from "@wordpress/element";
import { BlockConfiguration, BlockInstance } from "@wordpress/blocks";
import { BlockValidationResult, BlockValidation } from "../validation";
import { BlockValidation, BlockValidationResult } from "../validation";
import Instruction, { InstructionOptions } from "../Instruction";
import { attributeExists, attributeNotEmpty } from "../../functions/validators";
import validateMany from "../../functions/validators/validateMany";
import logger from "../../functions/logger";
import { BlockPresence } from "../validation/BlockValidationResult";

export type BlockInstructionClass = {
new( id: number, options: InstructionOptions ): BlockInstruction;
Expand Down Expand Up @@ -78,16 +79,18 @@ export default abstract class BlockInstruction extends Instruction {
* @returns {BlockValidationResult} The validation result.
*/
validate( blockInstance: BlockInstance ): BlockValidationResult {
const validation = new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Skipped );
const validation = new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Skipped, BlockPresence.Unknown );

if ( this.options && this.options.required === true ) {
const attributeValid = attributeExists( blockInstance, this.options.name as string ) &&
attributeNotEmpty( blockInstance, this.options.name as string );
if ( attributeValid ) {
validation.issues.push( new BlockValidationResult( blockInstance.clientId, this.options.name, BlockValidation.Valid ) );
validation.issues.push( new BlockValidationResult( blockInstance.clientId, this.options.name,
BlockValidation.Valid, BlockPresence.Unknown ) );
} else {
logger.warning( "block " + blockInstance.name + " has a required attributes " + this.options.name + " but it is missing or empty" );
validation.issues.push( new BlockValidationResult( blockInstance.clientId, this.options.name, BlockValidation.MissingAttribute ) );
validation.issues.push( new BlockValidationResult( blockInstance.clientId, this.options.name,
BlockValidation.MissingAttribute, BlockPresence.Unknown ) );
validation.result = BlockValidation.Invalid;
}
} else {
Expand Down
5 changes: 3 additions & 2 deletions packages/schema-blocks/src/core/schema/SchemaInstruction.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { SchemaValue, SchemaDefinitionConfiguration } from "./SchemaDefinition";
import { SchemaDefinitionConfiguration, SchemaValue } from "./SchemaDefinition";
import Instruction, { InstructionOptions } from "../Instruction";
import { BlockInstance } from "@wordpress/blocks";
import { BlockValidation, BlockValidationResult } from "../validation";
import { BlockPresence } from "../validation/BlockValidationResult";

export type SchemaInstructionClass = { new( id: number, options: InstructionOptions ): SchemaInstruction };

Expand Down Expand Up @@ -39,6 +40,6 @@ export default abstract class SchemaInstruction extends Instruction {
* @returns {BlockValidationResult} The validation results.
*/
validate( blockInstance: BlockInstance ): BlockValidationResult {
return new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Valid );
return new BlockValidationResult( blockInstance.clientId, blockInstance.name, BlockValidation.Valid, BlockPresence.Unknown );
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

export enum BlockValidation {
/** This block was skipped during validation, on purpose. If you ever see this value, that's a bug. */
Skipped = -2,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import { BlockValidation } from ".";
import { BlockInstance } from "@wordpress/blocks";

export enum BlockPresence {
Required = "required",
Recommended = "recommended",
Optional = "optional",
Unknown = "unknown"
}

/**
* Contains the result of a block validation.
*/
Expand All @@ -16,24 +23,31 @@ export class BlockValidationResult {
public clientId: string;

/**
* The validation result;
* The validation result.
*/
public result: BlockValidation;

/**
* The block presence.
*/
public blockPresence: BlockPresence;

/**
* The validation issues for this block's innerblocks or attributes, if any.
*/
public issues: BlockValidationResult[]

/**
* @param clientId The clientId of the validated block.
* @param name The name of the validated block.
* @param result The validation result.
* @param clientId The clientId of the validated block.
* @param name The name of the validated block.
* @param result The validation result.
* @param blockPresence The block type.
*/
constructor( clientId: string, name: string, result: BlockValidation ) {
this.name = name;
constructor( clientId: string, name: string, result: BlockValidation, blockPresence: BlockPresence ) {
this.clientId = clientId;
this.name = name;
this.result = result;
this.blockPresence = blockPresence;
this.issues = [];
}

Expand All @@ -50,6 +64,7 @@ export class BlockValidationResult {
blockInstance.clientId,
name || blockInstance.name,
BlockValidation.MissingAttribute,
BlockPresence.Unknown,
);
}

Expand All @@ -66,6 +81,7 @@ export class BlockValidationResult {
blockInstance.clientId,
name || blockInstance.name,
BlockValidation.Valid,
BlockPresence.Unknown,
);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { RequiredBlockOption } from ".";
import { InstructionOptions } from "../Instruction";
import { RequiredBlockOption } from ".";
import { SuggestedBlockProperties } from "./SuggestedBlockProperties";

/**
Expand Down
3 changes: 2 additions & 1 deletion packages/schema-blocks/src/functions/gutenberg/watch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { getBlockDefinition } from "../../core/blocks/BlockDefinitionRepository"
import { BlockValidation, BlockValidationResult } from "../../core/validation";
import storeBlockValidation from "./storeBlockValidation";
import logger from "../logger";
import { BlockPresence } from "../../core/validation/BlockValidationResult";

let updatingSchema = false;
let previousRootBlocks: BlockInstance[];
Expand Down Expand Up @@ -104,7 +105,7 @@ export function validateBlocks( blocks: BlockInstance[] ): BlockValidationResult
validations.push( definition.validate( block ) );
} else {
logger.warning( "Unable to validate block of type [" + block.name + "] " + block.clientId );
validations.push( new BlockValidationResult( block.clientId, block.name, BlockValidation.Unknown ) );
validations.push( new BlockValidationResult( block.clientId, block.name, BlockValidation.Unknown, BlockPresence.Unknown ) );

// Recursively validate all blocks' innerblocks.
if ( block.innerBlocks && block.innerBlocks.length > 0 ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { SvgIcon } from "@yoast/components";
* Renders warnings and appenders for any block's InnerBlocks.
*
* @param currentBlock The innerblocks instance.
* @param options The InnerBlocksInstructionOptions of the innerblock.
* @param options The InnerBlocksInstructionOptions of the innerblock.
*
* @returns {ReactElement[]} React Elements representing the sidebar elements for these innerblocks.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import { __ } from "@wordpress/i18n";
import { get } from "lodash";
import { BlockValidationResult } from "../../core/validation";
import { BlockValidation } from "../../core/validation";
import { BlockPresence } from "../../core/validation/BlockValidationResult";

const analysisMessageTemplates: Record<number, string> = {
[ BlockValidation.MissingBlock ]: "The '{child}' block is {status} but missing.",
[ BlockValidation.MissingBlock ]: "The '{child}' block is {presence} but missing.",
[ BlockValidation.MissingAttribute ]: "The '{child}' block is empty.",
};

Expand All @@ -16,7 +17,7 @@ type analysisIssue = {
name: string;
parent: string;
result: BlockValidation;
status: string;
presence: string;
};

export type sidebarWarning = {
Expand Down Expand Up @@ -51,7 +52,7 @@ export function replaceVariables( issue: analysisIssue ): string {
const warning = get( analysisMessageTemplates, issue.result, "" );
return warning.replace( "{parent}", __( issue.parent, "wpseo-schema-blocks" ) )
.replace( "{child}", __( issue.name, "wpseo-schema-blocks" ) )
.replace( "{status}", __( issue.status, "wpseo-schema-blocks" ) );
.replace( "{presence}", __( issue.presence, "wpseo-schema-blocks" ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this will cause merge conflicts with #1127 where this logic (replaceVariables) has been removed.

Also please not that dynamic arguments for translations aren't translatable. The potfile builder will take issue.presence as value to translate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know!

}

/**
Expand All @@ -63,20 +64,26 @@ export function replaceVariables( issue: analysisIssue ): string {
* @returns {sidebarWarning} Any analysis conclusions that should be in the footer.
*/
function getAnalysisConclusion( validation: BlockValidation, issues: analysisIssue[] ): sidebarWarning {
if ( issues.some( issue => issue.result === BlockValidation.MissingBlock ||
// Show a red bullet when not all required blocks have been completed.
if ( issues.some( issue => issue.result === BlockValidation.MissingBlock && issue.presence === BlockPresence.Required ||
issue.result === BlockValidation.MissingAttribute ) ) {
return {
text: __( "Not all required blocks are completed! No " + issues[ 0 ].parent +
text: __( "Not all required blocks have been completed! No " + issues[ 0 ].parent +
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic has also been changed in #1127

" schema will be generated for your page.", "wpseo-schema-blocks" ),
color: "red",
} as sidebarWarning;
}

// Show a green bullet when all required blocks have been completed.
const requiredBlockIssues = issues.filter( issue => {
return issue.presence === BlockPresence.Required;
} );

if ( validation === BlockValidation.Valid ||
issues.every( issue => issue.result !== BlockValidation.MissingAttribute &&
requiredBlockIssues.every( issue => issue.result !== BlockValidation.MissingAttribute &&
issue.result !== BlockValidation.MissingBlock ) ) {
return {
text: __( "Good job! All required blocks are completed.", "wpseo-schema-blocks" ),
text: __( "Good job! All required blocks have been completed.", "wpseo-schema-blocks" ),
color: "green",
} as sidebarWarning;
}
Expand All @@ -87,7 +94,7 @@ function getAnalysisConclusion( validation: BlockValidation, issues: analysisIss
*
* @param validation The root validation result.
*
* @return all validation results.
* @return All validation results.
*/
function getAllDescendantIssues( validation: BlockValidationResult ): BlockValidationResult[] {
let results = [ validation ];
Expand All @@ -114,10 +121,11 @@ export function createAnalysisMessages( validation: BlockValidationResult ): sid
name: sanitizeBlockName( issue.name ),
parent: sanitizeParentName( parent ),
result: issue.result,
status: "required",
presence: issue.blockPresence,
} ) );

const messages = messageData.map( msg => {
return { text: replaceVariables( msg ), color: "red" } as sidebarWarning;
return { text: replaceVariables( msg ), color: ( msg.presence === BlockPresence.Recommended ? "orange" : "red" ) } as sidebarWarning;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip: I would make this more readable by adding newlines. Especially the conditional makes this all hard to read/process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. But I believe this code has been changed in #1127 anyway.

} );

const conclusion = getAnalysisConclusion( validation.result, messageData );
Expand Down
Loading