feat[react-gen2]: support for variant containers#3828
feat[react-gen2]: support for variant containers#3828sidmohanty11 merged 52 commits intoBuilderIO:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 90620c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit 90620c7.
☁️ Nx Cloud last updated this comment at |
| // Only remove the function if it's not directly exported | ||
| const exportParent = fnNode.findParent((p) => | ||
| p.isExportNamedDeclaration() | ||
| ); | ||
| if ( | ||
| !exportParent || | ||
| !t.isFunctionDeclaration(exportParent.node.declaration) | ||
| ) { | ||
| fnNode.remove(); | ||
| } |
There was a problem hiding this comment.
@samijaber had to update this file to allow exporting fns if they are marked exported. This was needed because we cannot convert to a fn from a string in hydrogen because of their strict policy (in the server) that we cannot use eval/Function, i.e, earlier I was trying this
export const filterWithCustomTargeting = new Function(
`return ${FILTER_WITH_CUSTOM_TARGETING_SCRIPT}`
)();
|
@samijaber thanks for the build-inline-fns idea, it simplified the logic a lot. Also have added the tests for the |
samijaber
left a comment
samijaber
left a comment
There was a problem hiding this comment.
I'm unable to drag'n'drop successfully into an empty Variant Container block
|
There may be an issue with how the previewingIndex is applied, unless I'm confused: https://www.loom.com/share/af8e458536574721b83b456dba695a2f I can only see and edit variant 1 in the visual editor |
| const thisPrefix = 'this.'; | ||
| const pathPrefix = 'component.options.'; | ||
| return props.path.startsWith(pathPrefix) | ||
| ? props.path | ||
| : `${pathPrefix}${props.path || ''}`; | ||
| return props.path.startsWith(thisPrefix) | ||
| ? props.path.replace(thisPrefix, '') | ||
| : props.path.startsWith(pathPrefix) | ||
| ? props.path | ||
| : `${pathPrefix}${props.path || ''}`; |
There was a problem hiding this comment.
Thanks for the catch!
added children support for <Blocks /> for this
I'm unable to drag'n'drop successfully into an empty Variant Container block
we need to render them as <Blocks /> because:
- we need the BlocksWrapper to render so that hydration matches with the other variants
- this will allow us to drag and drop - will enable empty blocks
| return { | ||
| blocks: variant.blocks, | ||
| path: `component.options.variants.${previewingIndex}.blocks`, | ||
| }; | ||
| } | ||
| // Otherwise we're editing the default variant | ||
| return fallback; | ||
| } | ||
|
|
||
| // If not editing, check if there's a winning variant | ||
| const winningVariant = filteredVariants?.[0]; | ||
| return winningVariant | ||
| ? { | ||
| blocks: winningVariant.blocks, | ||
| path: `component.options.variants.${variants?.indexOf(winningVariant)}.blocks`, | ||
| } | ||
| : // else return the default variant | ||
| fallback; | ||
| } |
There was a problem hiding this comment.
There may be an issue with how the previewingIndex is applied, unless I'm confused:
Turns out this was happening because when we don't provide any targeting, filteredVariants renders a winning variant. Have fixed this
| test('only default variants are ssred on the server', async ({ browser, packageName, sdk }) => { | ||
| test.skip(!isSSRFramework(packageName)); | ||
| test.skip(!['react', 'oldReact'].includes(sdk)); | ||
| // Cannot read properties of null (reading 'useContext') |
There was a problem hiding this comment.
@samijaber we should create a ticket to move away from remix v1 for gen1 tests and use v2 that we already have in snippets. This will help us alias a single react and fix these tests
repo: https://github.com/sidmohanty11/builder-remix-gen1-example
There was a problem hiding this comment.
yeah @sidmohanty11 , I actually spent a bunch of time doing this locally but hit some cascading issues that caused most (interactivity-related?) tests to fail for remix v2, so i ended up putting a pin in it for now. But would be good to sort it out
samijaber
left a comment
samijaber
left a comment
There was a problem hiding this comment.
Another bug I found: styles (probably other root-level attributes too) not applying to root VariantContainer itself: https://www.loom.com/share/8576990685a7420f8fa33cfbac9a6ce2
|
@samijaber thanks for the catch again! it was for styles only, the block didn't have the builder-id in the classname so it wasn't getting applied. Have fixed this and added a test for this
|
2 participants
Description
Adds support for Variant Containers on Gen2 React SDK
Jira
https://builder-io.atlassian.net/browse/ENG-7676
Screenshot
If relevant, add a screenshot or two of the changes you made.