Skip to content
This repository has been archived by the owner on Oct 4, 2022. It is now read-only.

P2 786 only render ok when block is valid #1146

Merged
merged 41 commits into from
May 10, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
4f15ce7
Added a method that checks if the blockValidationResult is considered…
Apr 7, 2021
d976c74
Adds a check to see if the added suggestion is valid
Apr 7, 2021
0e3a803
Updates the tests for the blocksuggestions
Apr 7, 2021
f772213
Improval of the test by mocking the functions directly
Apr 7, 2021
bc2a734
Merge branch 'develop' of https://github.com/Yoast/javascript into P2…
Apr 8, 2021
9eb9930
Process CR
Apr 8, 2021
c467972
Remove typehints in jsdoc
Apr 8, 2021
54dfb3a
Merge branch 'develop' of https://github.com/Yoast/javascript into P2…
Apr 8, 2021
cd2ea53
Check if the block validation is OK
Apr 12, 2021
c67c29a
Merge branch 'develop' of https://github.com/Yoast/javascript into P2…
Apr 12, 2021
ebfa7e5
Merge branch 'develop' of https://github.com/Yoast/javascript into P2…
Apr 14, 2021
8e2debd
Refactored the BlockSuggestionsPresenter a little bit
Apr 15, 2021
7404075
Changes the working of the validation a little bit.
Apr 15, 2021
51d120d
make the SidebarWarningPresenter retrieve validation for a client Id
increddibelly Apr 15, 2021
74d36e4
fix unit tests
increddibelly Apr 16, 2021
c89071d
extract the BlockSuggestionsPresenter logic from the component
increddibelly Apr 16, 2021
e95a228
BlockSuggestionsPresenter allows more detailed control now; validatio…
increddibelly Apr 19, 2021
565f3fe
Merge branch 'develop' into P2-786-only-render-ok-when-block-is-valid
increddibelly Apr 19, 2021
0426ce0
added additional valid results and validation
increddibelly Apr 20, 2021
9c10dcf
Add validation to RichtTextBase
increddibelly Apr 20, 2021
e251ab9
remove unused code
increddibelly Apr 20, 2021
25356ce
added ValidationBlockInstruction to allow blocks to opt-in validation
increddibelly Apr 23, 2021
9872f78
fixing unit tests
increddibelly Apr 23, 2021
cd31dfe
fixed unit tests
increddibelly Apr 29, 2021
94a719f
Bump @wordpress/data to the latest version
johannadevos Apr 29, 2021
87a09ca
prevent an error for missing instructions
increddibelly Apr 29, 2021
f9f5aaf
Merge remote-tracking branch 'origin/release/16.3' into P2-786-only-r…
increddibelly Apr 29, 2021
cad6799
replaced ValidatingBlockInstruction base class with single function
increddibelly Apr 30, 2021
9f8404e
Merge remote-tracking branch 'origin/release/16.3' into P2-786-only-r…
increddibelly Apr 30, 2021
1d0e61d
fine tune variationpicker validation
increddibelly May 3, 2021
a41779b
fix Date validation
increddibelly May 3, 2021
3c14f53
typo
increddibelly May 3, 2021
e28dcb0
Merge remote-tracking branch 'origin/develop' into P2-786-only-render…
increddibelly May 3, 2021
8db91db
fix Date tests.
increddibelly May 4, 2021
318b5a3
Update packages/schema-blocks/src/functions/presenters/BlockSuggestio…
increddibelly May 6, 2021
0f4d7b1
Update packages/schema-blocks/src/functions/presenters/BlockSuggestio…
increddibelly May 6, 2021
94fa03a
processed CR comments
increddibelly May 6, 2021
c93634a
Update packages/schema-blocks/src/core/validation/BlockValidationResu…
increddibelly May 6, 2021
3cfa7f1
fix CS
increddibelly May 6, 2021
a92916b
make method of reading current block content consistent with other Yo…
increddibelly May 6, 2021
0053d33
Removed superfluous isValid check from BlockSuggestionsPresenter.
hansjovis May 6, 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
Prev Previous commit
Next Next commit
make the SidebarWarningPresenter retrieve validation for a client Id
  • Loading branch information
increddibelly committed Apr 15, 2021
commit 51d120d7c29160d6e20002453f295c7922587986
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { ReactElement } from "react";
import { BlockInstance, createBlock } from "@wordpress/blocks";
import { PanelBody } from "@wordpress/components";
import { BlockPresence, BlockValidation, BlockValidationResult } from "../../core/validation";
import { getValidationResult } from "../validators";
import { getValidationResults, getValidationResultForClientId } from "../validators";
import { createElement } from "@wordpress/element";
import { getBlockType } from "../BlockHelper";
import { getInnerblocksByName, insertBlock } from "../innerBlocksHelper";
Expand Down Expand Up @@ -70,30 +69,7 @@ function BlockSuggestionAdded( { suggestedBlockTitle, isValid }: BlockSuggestion
</li>
);
}
/**
* Finds a blockInstance's validation result in a tree of validation results based on the block's clientId.
* @param clientId The ClientId of the block you want validation results for.
* @param validationResults The ValidationResult tree to investigate.
* @returns The BlockValidationResult matching the clientId or null if none were found.
*/
function GetValidationForClientId( clientId: string, validationResults: BlockValidationResult[] ): BlockValidationResult {
for ( const validationResult of validationResults ) {
// When the validation result matches the client id. Just return it.
if ( validationResult.clientId === clientId ) {
return validationResult;
}

// Just keep it calling until we have found a result.
if ( validationResult.issues.length > 0 ) {
const validation = GetValidationForClientId( clientId, validationResult.issues );
if ( validation ) {
return validation;
}
}
}

return null;
}
/**
* Renders a list of suggested blocks.
*
Expand All @@ -112,36 +88,32 @@ export default function BlockSuggestionsPresenter( { heading, parentBlock, sugge
}

const presentBlocks = getInnerblocksByName( parentBlock, filteredSuggestedBlockNames );
const validationResult = getValidationResult( parentBlock.clientId );
let validationIssues: BlockValidationResult[] = [];
if ( validationResult ) {
validationIssues = validationResult.issues;
}
const validationResults = getValidationResults();

return (
<PanelBody key={ heading + parentBlock.clientId }>
<div className="yoast-block-sidebar-title">{ heading }</div>
<div className="yoast-block-sidebaryarn lint-fix-title">{ heading }</div>
<ul className="yoast-block-suggestions">
{
filteredSuggestedBlockNames.map( ( suggestedBlockName: string, index: number ) => {
const suggestedBlockType = getBlockType( suggestedBlockName );

const existingBlock = presentBlocks.find( block => block.name === suggestedBlockName );
if ( existingBlock ) {
const validation = GetValidationForClientId( existingBlock.clientId, validationIssues ) ||
new BlockValidationResult(
existingBlock.clientId,
existingBlock.name,
BlockValidation.Unknown,
BlockPresence.Unknown );
// Find the validation result for this block.
const validation = getValidationResultForClientId( existingBlock.clientId, validationResults );
// If the validation was found, and it is completely OK, we will add a check mark.
const isTheBlockValid = validation && isValidResult( validation.result );

const isTheBlockValid = isValidResult( validation.result );
// Show the validation result.
return <BlockSuggestionAdded
key={ index }
isValid={ isTheBlockValid }
suggestedBlockTitle={ suggestedBlockType.title }
/>;
}

// Show the suggestion to add an instance of this block.
return <BlockSuggestion
key={ index }
suggestedBlockTitle={ suggestedBlockType.title }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { __, sprintf } from "@wordpress/i18n";
import { BlockValidation, BlockValidationResult } from "../../core/validation";
import { getHumanReadableBlockName } from "../BlockHelper";
import { BlockPresence } from "../../core/validation/BlockValidationResult";
import { getAllDescendantIssues, getValidationResult } from "../validators";
import { getAllDescendantIssues } from "../validators";

/**
* A warning message for in the sidebar schema analysis.
Expand Down Expand Up @@ -124,19 +124,3 @@ export function sanitizeParentName( parent: string ): string {

return parent.toLowerCase();
}

/**
* Converts the validation results for a block instance with the given clientId to a presentable text.
*
* @param clientId The clientId to request validation results for.
*
* @returns {string} The presentable warning message, or null if no warnings are found.
*/
export default function getWarnings( clientId: string ): SidebarWarning[] {
const validation: BlockValidationResult = getValidationResult( clientId );
if ( ! validation ) {
return null;
}

return createAnalysisMessages( validation );
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,44 @@ type clientIdValidation = Record<string, BlockValidationResult>;
*
* @param clientId The clientId to request validation results for.
*
* @returns {BlockValidationResult} The validation results, or null if none were found.
* @returns The validation results, or null if none were found.
*/
export default function getValidationResult( clientId: string ): BlockValidationResult | null {
export function getValidationResults(): BlockValidationResult[] | null {
const validationResults: clientIdValidation = select( YOAST_SCHEMA_BLOCKS_STORE_NAME ).getSchemaBlocksValidationResults();
if ( ! validationResults ) {
return null;
}

return validationResults[ clientId ];
const validations: BlockValidationResult[] = Object.values( validationResults );
return validations;
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

The validations intermediate variable is not necessary.

Suggested change
const validations: BlockValidationResult[] = Object.values( validationResults );
return validations;
return Object.values( validationResults );

}

/**
* Recursively traverses a BlockValidationResult's issues to finds the validation results for a specific clientId.
* @param clientId The ClientId of the block you want validation results for.
* @param validationResults The (partial) ValidationResult tree to investigate; reads the entire tree from the store by default.
* @returns The BlockValidationResult matching the clientId or null if none were found.
*/
export function getValidationResultForClientId( clientId: string, validationResults?: BlockValidationResult[] ): BlockValidationResult {
if ( ! validationResults ) {
validationResults = getValidationResults();
}

for ( const validationResult of validationResults ) {
// When the validation result matches the client id, return it.
if ( validationResult.clientId === clientId ) {
return validationResult;
}

// Just keep driving down the tree calling until we have found the result.
if ( validationResult.issues.length > 0 ) {
const validation = getValidationResultForClientId( clientId, validationResult.issues );
if ( validation ) {
return validation;
}
}
}

// We haven't found the result down this tree.
return null;
}
5 changes: 3 additions & 2 deletions packages/schema-blocks/src/functions/validators/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import attributeExists from "./attributeExists";
import attributeNotEmpty from "./attributeNotEmpty";
import getAllDescendantIssues from "./getAllDescendantIssues";
import getInvalidInnerBlocks from "./innerBlocksValid";
import getValidationResult from "./getValidationResult";
import { getValidationResults, getValidationResultForClientId } from "./getValidationResult";

export {
attributeExists,
attributeNotEmpty,
getAllDescendantIssues,
getInvalidInnerBlocks,
getValidationResult,
getValidationResults,
getValidationResultForClientId,
};
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jest.mock( "@wordpress/blocks", () => {

jest.mock( "../../../src/functions/validators", () => {
return {
getValidationResult: jest.fn( () => {
getValidationResults: jest.fn( () => {
return new BlockValidationResult( "1", "yoast/valid-block", -1, BlockPresence.Required, "Is not that present" );
} ),
getAllDescendantIssues: jest.fn( () => {
Expand All @@ -34,7 +34,7 @@ jest.mock( "../../../src/functions/BlockHelper", () => {
}

return {
title: "The required block",
heading: "The required block",
};
} ),
};
Expand All @@ -47,6 +47,7 @@ jest.mock( "../../../src/functions/innerBlocksHelper", () => {
return [
{
name: "yoast/added-to-content",
clientId: "existingBlockClientId",
},
];
} ),
Expand All @@ -55,41 +56,41 @@ jest.mock( "../../../src/functions/innerBlocksHelper", () => {

describe( "The required blocks in the sidebar", () => {
it( "doesn't have the required block being registered as a block", () => {
const block = { innerBlocks: [] } as BlockInstance;
const parentBlock = { innerBlocks: [] } as BlockInstance;
const requiredBlocks = [ "yoast/nonexisting" ];

const actual = BlockSuggestionsPresenter( { title: "Required blocks", block, suggestions: requiredBlocks } );
const actual = BlockSuggestionsPresenter( { heading: "Required blocks", parentBlock, suggestedBlockNames: requiredBlocks } );

expect( actual ).toBe( null );
} );

it( "renders the required block as an added one", () => {
const block = { innerBlocks: [] } as BlockInstance;
const parentBlock = { innerBlocks: [] } as BlockInstance;
const requiredBlocks = [ "yoast/added-to-content" ];

const tree = renderer
.create( BlockSuggestionsPresenter( { title: "Required blocks", block, suggestions: requiredBlocks } ) )
.create( BlockSuggestionsPresenter( { heading: "Required blocks", parentBlock, suggestedBlockNames: requiredBlocks } ) )
.toJSON();

expect( tree ).toMatchSnapshot();
} );

it( "renders the required block as a non-added one", () => {
const block = { innerBlocks: [] } as BlockInstance;
const parentBlock = { innerBlocks: [] } as BlockInstance;
const requiredBlocks = [ "yoast/non-added-to-content" ];

const tree = renderer
.create( BlockSuggestionsPresenter( { title: "Required blocks", block, suggestions: requiredBlocks } ) )
.create( BlockSuggestionsPresenter( { heading: "Required blocks", parentBlock, suggestedBlockNames: requiredBlocks } ) )
.toJSON();

expect( tree ).toMatchSnapshot();
} );

it( "should call the function to add the block when the button is clicked.", () => {
const block = { innerBlocks: [], clientId: "1" } as BlockInstance;
const parentBlock = { innerBlocks: [], clientId: "1" } as BlockInstance;
const requiredBlocks = [ "yoast/non-added-to-content" ];

const tree = mount( BlockSuggestionsPresenter( { title: "Required blocks", block, suggestions: requiredBlocks } ) );
const tree = mount( BlockSuggestionsPresenter( { heading: "Required blocks", parentBlock, suggestedBlockNames: requiredBlocks } ) );

const addButton = tree.find( "button" ).first();

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { BlockValidation, BlockValidationResult } from "../../../src/core/validation";
import getWarnings, { createAnalysisMessages } from "../../../src/functions/presenters/SidebarWarningPresenter";
import { createAnalysisMessages } from "../../../src/functions/presenters/SidebarWarningPresenter";
import { BlockPresence } from "../../../src/core/validation/BlockValidationResult";
import { BlockInstance } from "@wordpress/blocks";

Expand Down Expand Up @@ -133,56 +133,4 @@ describe( "The SidebarWarningPresenter ", () => {
} );
} );
} );

describe( "The getWarnings method ", () => {
it( "creates a compliment for required valid blocks.", () => {
validations[ "1" ] = new BlockValidationResult( "1", "myBlock", BlockValidation.Valid, BlockPresence.Required );

const result = getWarnings( "1" );

expect( result ).toEqual( [ {
text: "Good job! All required blocks have been completed.",
color: "green",
} ] );
} );

it( "creates a compliment if we do not have copy for any of the validations of the required blocks.", () => {
const testcase = new BlockValidationResult( "1", "myBlock", BlockValidation.Invalid, BlockPresence.Required );
testcase.issues.push( new BlockValidationResult( "2", "innerblock1", BlockValidation.Skipped, BlockPresence.Required ) );
testcase.issues.push( new BlockValidationResult( "3", "anotherinnerblock", BlockValidation.TooMany, BlockPresence.Required ) );
testcase.issues.push( new BlockValidationResult( "4", "anotherinnerblock", BlockValidation.Unknown, BlockPresence.Required ) );
validations[ "1" ] = testcase;

const result = getWarnings( "1" );

expect( result ).toEqual( [ {
text: "Good job! All required blocks have been completed.",
color: "green",
} ] );
} );

it( "creates a warning for a required block with validation problems.", () => {
const testcase = new BlockValidationResult( "1", "myBlock", BlockValidation.Invalid, BlockPresence.Required );
testcase.issues.push( BlockValidationResult.MissingBlock( "innerblock1", BlockPresence.Required ) );
validations[ "1" ] = testcase;

const result = getWarnings( "1" );

expect( result.length ).toEqual( 2 );
expect( result[ 0 ] ).toEqual( {
text: "The `innerblock1` block is required but missing.",
color: "red",
} );
expect( result[ 1 ] ).toEqual( {
text: "Not all required blocks have been completed! No myblock schema will be generated for your page.",
color: "red",
} );
} );
} );

it( "creates no output when the validation results cannot be retrieved.", () => {
const result = getWarnings( "123" );

expect( result ).toBeNull();
} );
} );