feat: render prebinding data in preview mode [SPA-2923] #1326
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Spring3
changed the title
Spring3
force-pushed
the
feat/preview-render-prebinding-data
branch
from
0ff6e60 to
3b3f3af
Compare
Spring3
requested review from
duruld and
Chaoste
and removed request for
a team
Spring3
force-pushed
the
feat/preview-render-prebinding-data
branch
from
52a41c3 to
06403bc
Compare
| @@ -0,0 +1,547 @@ | |||
| import { | |||
There was a problem hiding this comment.
Would it make sense to keep a minimal version of the prebindingManager that only includes:
storePrebindingDefinitionsstoreParameterDefinitionsgetHoistedParameterByNodeId
Given that the one in the Web App has already diverged from this one, it just makes things more complicated to keep them in sync.
There was a problem hiding this comment.
yes, it makes sense 👍
There was a problem hiding this comment.
I will align it with the latest version from the web app I think
* fix: create general function to extract slot and direct child nodes for both editor and preview * test: add fixtures and test for new core util function * feat: add example to test-apps for testing hardcoded width and children + slots
Spring3
force-pushed
the
feat/preview-render-prebinding-data
branch
from
06403bc to
8b9d25e
Compare
| pattern: { | ||
| nodeIdOnPattern: 'test-custom-component-id', | ||
| parentPatternNodeId: undefined, | ||
| prefixedNodeId: 'test-custom-component-id', | ||
| }, |
There was a problem hiding this comment.
wanted to point out that I decided to keep these properties just in case, but they aren't used anywhere. So we can clean them up. I'm happy to hear your opinions
| * @param indexedNodeId - The indexed node ID to resolve the parameterId for | ||
| * @returns string - The resolved parameterId - will be from "root" in patterns or the parent pattern node in experiences | ||
| */ | ||
| public static getHoistedIdForParameterId(parameterId: string, indexedNodeId: string): string { |
There was a problem hiding this comment.
Do we not need the latest version with the fix applied from this morning?
There was a problem hiding this comment.
I don't think so, because indexing here was done slightly differently (and here.) we purely pass the pattern node ids chain joined with a separator and never actually pass the regular node ids
so getHoistedIdForParameterId is always called with the pattern ids and never with the component node ids. shouldUsePrebinding checks if the component's property appears in the native variableMapping and if it does - it takes its value by the hoisted parameter id
There was a problem hiding this comment.
Since we don't have the editor functionality at this point, we don't care of the component node ids on the rendering path
There was a problem hiding this comment.
Makes sense. Hopefully we don't need to touch it much but the fact they are mostly identical could get confusing why/when they should diverge.
| const patternRootNodeIdsChain = useMemo(() => { | ||
| if (isPatternNode) { | ||
| // Pattern nodes are chained without a separator (following the format for prebinding/parameters) | ||
| return `${parentPatternRootNodeIdsChain}${rawNode.id}`; | ||
| return [...parentPatternRootNodeIdsChain, rawNode.id!]; | ||
| } | ||
| return parentPatternRootNodeIdsChain; | ||
| }, [isPatternNode, parentPatternRootNodeIdsChain, rawNode.id]); |
There was a problem hiding this comment.
this is what is used in getHoistedIdForParameterId method and as you can see, we include the new id only if it's a nested pattern.
Since each node of the pattern has it's own unique id in the layout, we don't get a collision when having 2 instances of the same nested pattern.
We should migrate to use node ids as well on the web app side
| return; | ||
| } | ||
|
|
||
| const fieldPath = variableMapping.pathsByContentType[contentTypeId].path; |
There was a problem hiding this comment.
It's not in your changes, but this will fail if there is no mapping for a specific CT. It's the scenario where I am mapping:
CT-Ato:- propA
- propB
CT-Bto:- prop C
And my selected source is of type CT-A. When resolving mapping for propC, variableMapping.pathsByContentType[contentTypeId] will be undefined and this will throw. Adding some optional chaining should cover this case.
It would be nice if someone would cover it in a follow-up. Let's drop a ticket for visibility |
No worries, I will open a quick PR |
Purpose
Approach