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

P2-704 Same Post and Job titles do not trigger red analysis #1120

Closed
wants to merge 13 commits into from

Conversation

hansjovis
Copy link
Contributor

@hansjovis hansjovis commented Mar 25, 2021

Summary

This PR can be summarized in the following changelog entry:

  • [schema-blocks] Reworks the imports and exports of the schema blocks package.

Relevant technical choices:

  • Decided together with @increddibelly to rework the imports and exports.
  • Reworked the warning messages in the sidebar.
    • Now gets the warnings from the BlockValidationResults itself, rather than a separate result -> warning map.

Test instructions

This PR can be tested by following these steps:

Impact check

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Fixes #

@hansjovis hansjovis added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Mar 25, 2021
Copy link
Contributor

@increddibelly increddibelly left a comment

Choose a reason for hiding this comment

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

only a few comments for minor improvements.

also, /src/functions/blocks/index.ts is an empty file, please remove

@@ -1,7 +1,7 @@
import { ReactElement } from "react";
import { createElement } from "@wordpress/element";
import { BlockInstance } from "@wordpress/blocks";
import BlockSuggestions from "../../blocks/BlockSuggestions";
import { RequiredBlocks as BlockSuggestions } from "../../blocks/BlockSuggestions";
Copy link
Contributor

Choose a reason for hiding this comment

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

why the rename?

@@ -1,6 +1,6 @@
import attributeExists from "./attributeExists";
import attributeNotEmpty from "./attributeNotEmpty";
import getInvalidInnerBlocks from "./innerBlocksValid";
import { validateInnerBlocks as getInvalidInnerBlocks } from "./innerBlocksValid";
Copy link
Contributor

Choose a reason for hiding this comment

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

why the rename?

Suggested change
import { validateInnerBlocks as getInvalidInnerBlocks } from "./innerBlocksValid";
import { validateInnerBlocks } from "./innerBlocksValid";

@@ -6,6 +6,7 @@ import recurseOverBlocks from "../blocks/recurseOverBlocks";
import { getInnerblocksByName } from "../innerBlocksHelper";
import logger from "../logger";
import isValidResult from "./isValidResult";
Copy link
Contributor

Choose a reason for hiding this comment

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

there's still a default export remaining here.

@@ -3,10 +3,10 @@ import { BlockEditProps, BlockConfiguration } from "@wordpress/blocks";
import { createElement } from "@wordpress/element";
import { SelectControl } from "@wordpress/components";
// Internal imports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Internal imports.


import VariableTagRichText from "./VariableTagRichText";

export { VariableTagRichText };
Copy link
Contributor

Choose a reason for hiding this comment

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

why import the rest if we're not exporting them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the BlockInstruction.register side effects.

Without them the block instructions are not registered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yes, I will export them all.

import "./Schema";
import SchemaInstruction from "./Schema";

// Need to export something for the side-effects of the imports to work...
Copy link
Contributor

Choose a reason for hiding this comment

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

dank voor dit comment!
kunnen we de side effects expliciet maken / aanroepen?

@@ -1,13 +1,21 @@
import { dispatch, select } from "@wordpress/data";
import { BlockInstance } from "@wordpress/blocks";

import { removeBlock, restoreBlock, getBlockType } from "../../src/functions/BlockHelper";
import { removeBlock, restoreBlock, getBlockType, getHumanReadableBlockName } from "../../src/functions/BlockHelper";
Copy link
Contributor

Choose a reason for hiding this comment

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

rename the file from BlocksHelper.test.ts to BlockHelper.test.ts (singular)

@@ -1,8 +1,7 @@
import { BlockValidation, BlockValidationResult } from "../../../src/core/validation";
import getWarnings, { createAnalysisMessages, sanitizeBlockName } from "../../../src/functions/presenters/SidebarWarningPresenter";
import getWarnings, { createAnalysisMessages } from "../../../src/functions/presenters/SidebarWarningPresenter";
Copy link
Contributor

Choose a reason for hiding this comment

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

can we un-default export the getWarnings?

@hansjovis
Copy link
Contributor Author

Closed in favour of #1129.

@hansjovis hansjovis closed this Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants