-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
working on merge :( will finish monday |
Finished 💪 |
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; |
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.
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]; |
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.
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.
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:
SimpleUI
to choose between custom UI editors or a genericRichTextEditor
useVariables
,useBlocks
)BlockPreview
into separateBlockPreviewAdapter
filesisValid
state to avoid the saving on invalid intermediate states