Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 1334 Refactor Block Editor Flow #1337

Merged
merged 8 commits into from Jan 19, 2022

Conversation

greenrhyno
Copy link
Contributor

Closes #1334

Improves handling of branching BlockType-specific logic by making the delegation of responsibilities of various components related to block editing clearer.

Changes include:

  • simplification and consolidation of block content editing methods
  • expansion of SimpleUI to choose between custom UI editors or a generic RichTextEditor
  • reduction of passed props by use of shared selector hooks (e.g. useVariables, useBlocks)
  • reorganization of special BlockType-specific logic in BlockPreview into separate BlockPreviewAdapter files
  • addition of logic to allow editor components to set a isValid state to avoid the saving on invalid intermediate states
  • addition of JSDoc types for clearer function signatures

@greenrhyno
Copy link
Contributor Author

working on merge :( will finish monday

@greenrhyno
Copy link
Contributor Author

working on merge :( will finish monday

Finished 💪

@greenrhyno greenrhyno linked an issue Jan 18, 2022 that may be closed by this pull request
6 tasks
let payload = {};
// if a Block-specific preview adapter function exists, use that to build payload
if (PreviewAdapters[block.type] && typeof PreviewAdapters[block.type] === "function") {
payload = PreviewAdapters[block.type]({active, block, blockContent, locale, variables});
}
else if (block.type === BLOCK_TYPES.VIZ) {
payload.content = props;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that you were punting on understanding how Viz.jsx works, but you'll need to take a second look at it here.

Take a look at Viz.jsx and the arguments it receives - they no longer match the "props passthrough" that is occurring here, which is making Vizes not render.

The fix here is to make line 84 properly map its arguments into Viz.jsx's props. Viz is depended on by a number of other components (it's the same one used on the front-end) so it's better to change what we pass here than to modify viz to expect something different.

That said, a Viz re-write is forthcoming, but for now, just fix up the prop-passing here.

function RichTextEditor({locale, defaultContent, fields, onChange, variables, formatters, onTextModify}) {
function RichTextEditor({blockType, locale, defaultContent, id, onChange, onTextModify}) {

const fields = BLOCK_MAP[blockType];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate why you chose to move the field lookup inside rich text editor, it has a cleaner feel.

However, this makes the RichTextEditor strongly coupled with Blocks. It's arguable that everything in this entire project is strongly coupled with blocks (lol) so this may be a non-issue. But I figured it might be nice to have a "dumb" rich text editor around, where you pass it an arbitrary list of keys, and it builds an object for you.

I'll leave this call up to you, given that you are owning the whole SimpleUI/editor ecosystem at the moment. But just wanted to give you my thoughts.

packages/reports/src/utils/consts/cms.js Outdated Show resolved Hide resolved
@jhmullen jhmullen merged commit 39faa30 into cms-1.0-develop Jan 19, 2022
@jhmullen jhmullen deleted the issue-1334-refactor-block-flow branch January 19, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[reports] Refactor block component design for easier editor management
2 participants