This repository was archived by the owner on Oct 4, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 52
Show an orange bullet in the sidebar analysis when a recommended block is removed #1116
Merged
Merged
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
6d4b06e
Minor CS fixes
johannadevos abf4526
WIP: Add missing recommended blocks to the validation.issues in Inner…
johannadevos 3aea2f5
Add the BlockType enum to BlockValidationResult and, for the time bei…
johannadevos 99477c2
Rename the variables in findMissingBlocks to make the function inclus…
johannadevos 7031766
Pass the block type to the findMissingBlocks() function
johannadevos 8cfa903
Convert the numeric enum to a string enum
johannadevos 84014f8
Improve CS in imports
johannadevos 96af683
Improve naming in findMissingBlocks function
johannadevos b2edab2
Undo changes made to the RecommendedBlock type
johannadevos 47ceefc
Fix TS error where recommended blocks couldn't be passed to the findM…
johannadevos 40a0c40
Improve function documentation
johannadevos fa7fa29
Convert the enum values to lower case
johannadevos abc47c0
Show an orange bullet in the analysis if the missing block is recomme…
johannadevos c562969
Make sure the analysis conclusion is still shown in case there are re…
johannadevos b8a4546
Improve copy for the sidebar analysis
johannadevos bc81d23
Fix CS in imports
johannadevos 2a36ceb
Fix more CS in imports
johannadevos a99a09d
Improve documentation
johannadevos c3572ed
Fix eslint errors about the maximum length of a line
johannadevos 53fa3a3
Add the new BlockType parameter for the BlockValidationResult to exis…
johannadevos 248cd24
Add tests for recommended blocks
johannadevos 4f4e252
Test the scenario where the validation results cannot be retrieved
johannadevos acea646
Merge branch 'develop' of https://github.com/Yoast/javascript into P2…
johannadevos cbd2bba
Rename requiredIssues to requiredBlockIssues
johannadevos 5a558fe
Rename BlockType to BlockPresence
johannadevos 40260b6
Rename 'status' property in analysisIssue type to 'presence'
johannadevos dd6d261
Make "One" the default quantity for all the blocks; there is no longe…
johannadevos 0b0b5e2
Rename more occurrences of BlockType to BlockPresence
johannadevos 61804b6
Apply the `findRedundantBlocks` validation to the recommended blocks …
johannadevos 7281eab
Sort imports alphabetically
johannadevos 5e2faf6
Add the blockPresence parameter to the `innerBlocksValid` test
johannadevos 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 |
|---|---|---|
|
|
@@ -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.", | ||
| }; | ||
|
|
||
|
|
@@ -16,7 +17,7 @@ type analysisIssue = { | |
| name: string; | ||
| parent: string; | ||
| result: BlockValidation; | ||
| status: string; | ||
| presence: string; | ||
| }; | ||
|
|
||
| export type sidebarWarning = { | ||
|
|
@@ -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" ) ); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -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 + | ||
|
Contributor
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 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; | ||
| } | ||
|
|
@@ -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 ]; | ||
|
|
@@ -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; | ||
|
Contributor
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. Tip: I would make this more readable by adding newlines. Especially the conditional makes this all hard to read/process
Contributor
Author
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. Good point. But I believe this code has been changed in #1127 anyway. |
||
| } ); | ||
|
|
||
| const conclusion = getAnalysisConclusion( validation.result, messageData ); | ||
|
|
||
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.
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.presenceas value to translate.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.
Good to know!