feat: page block variables feature #2672
Conversation
|
To start the review, I pulled the branch and just started the app and clicked through. It's looking really good 😃 👏 Things I noticed that I'd like us to address:
CleanShot.2022-10-11.at.11.33.06.mp4I'll submit the second part of the review through actual code review, this feedback was easier to post this way. |
| for (const pageBlock of page.content?.elements) { | ||
| const blockId = pageBlock.data?.blockId; | ||
| // If block has blockId, then it is reference block and we need to get elements for it | ||
| if (blockId) { | ||
| const blockData = await context.pageBuilder.getPageBlock(blockId); | ||
| // We check if block has variable values set on the page and use them in priority over ones, | ||
| // that are set in blockEditor | ||
| const blockDataVariables = | ||
| blockData?.content?.data?.variables || []; | ||
| const variables = blockDataVariables.map( | ||
| (blockDataVariable: any) => { | ||
| const value = | ||
| pageBlock.data?.variables?.find( | ||
| (variable: any) => | ||
| variable.varRef === blockDataVariable.varRef | ||
| )?.value || blockDataVariable.value; | ||
|
|
||
| return { | ||
| ...blockDataVariable, | ||
| value | ||
| }; | ||
| } | ||
| ); | ||
|
|
||
| blocks.push({ | ||
| ...block, | ||
| data: { blockId, ...blockData?.content?.data }, | ||
| ...pageBlock, | ||
| data: { | ||
| blockId, | ||
| ...blockData?.content?.data, | ||
| variables | ||
| }, | ||
| elements: blockData?.content?.elements || [] | ||
| }); | ||
| } else { | ||
| blocks.push(block); | ||
| blocks.push(pageBlock); |
There was a problem hiding this comment.
Please extract this logic into a separate file, just so we keep the resolvers shorter and easier to read. Besides, you'll be able to unit test this mapping logic if it lives in a standalone function.
| formData => { | ||
| if (block && block.id) { | ||
| const newVariables = block.data?.variables?.map((variable: PbBlockVariable) => { | ||
| if (variable?.varRef === element?.data?.varRef) { |
There was a problem hiding this comment.
As I mentioned in the PR comment, let's use variableId within individual elements, instead of varRef.
| setTimeout(() => { | ||
| showSnackbar(`Block ${block.name} saved successfully!`); | ||
| showSnackbar(`Block "${block.name}" saved successfully!`); | ||
| history.push(`/page-builder/page-blocks`); | ||
| }, 500); |
There was a problem hiding this comment.
Just curious, why do we need a timeout here? This is not a change request, just want to know, because snackbar should be visible even after router change.
There was a problem hiding this comment.
@Pavel910 I have changed it, so setTimeout is not required anymore
but there are few more places where such approach is used:
packages/app-page-builder/src/pageEditor/config/editorBar/PublishPageButton/PublishPageButton.tsx line 45
packages/app-page-builder/src/pageEditor/config/editorBar/PageOptionsMenu/SetAsHomepageButton.tsx line 56
should I change them too?
There was a problem hiding this comment.
@neatbyte-vnobis To be honest, I think this is a leftover from a long time ago. If you can, please try removing it and see if it works. If not, leave it as is. Thanks a lot!
packages/app-page-builder/src/editor/plugins/elementSettings/clone/CloneAction.ts
Outdated
Show resolved
Hide resolved
|
@Pavel910 |
Changes
Block variables feature
How Has This Been Tested?
Manual
Documentation
None