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

fix: do not set width binding on Builder Column if value is undefined #1642

Merged
merged 8 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/soft-moons-change.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@builder.io/mitosis': patch
---

[Builder]: Do not set width binding on Column if value is undefined
6 changes: 3 additions & 3 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ We need your help! If you found a bug, it's best to [create an issue](https://gi

## Developing for Core & Testing

In `core`, we use vitest snapshots & integeration tests for test coverage. If you are solving a problem that is reproducible by a fiddle in [mitosis.builder.io/playground](/playground), we highly recommend the following flow:
In `core`, we use vitest snapshots & integration tests for test coverage. If you are solving a problem that is reproducible by a fiddle in [mitosis.builder.io/playground](/playground), we highly recommend the following flow:

### Snapshot test

Expand All @@ -55,8 +55,8 @@ PS: don't worry about failing imports in the raw test TSX files. These are not a

#### Create new e2e project for another target

If you want to create a new project inside ``e2e``. You should name the folder `e2e-XXX` where `XXX` should be replaced with the target.
Make sure that you change the ``name`` inside `package.json` of this project to `@builder.io/e2e-XXX`. Additionally, you need to add `private: true` to `package.json` to avoid publishing the project.
If you want to create a new project inside `e2e`. You should name the folder `e2e-XXX` where `XXX` should be replaced with the target.
Make sure that you change the `name` inside `package.json` of this project to `@builder.io/e2e-XXX`. Additionally, you need to add `private: true` to `package.json` to avoid publishing the project.

### Test your changes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ function MyComponent(props) {
paddingRight: \\"0px\\",
}}
>
<Column width={}>
<Column>
<Image
image=\\"https://cdn.builder.io/api/v1/image/assets%2FagZ9n5CUKRfbL9t6CaJOyVSK4Es2%2F8e096f01b00343dca3952d645f7ae024?width=998&height=1000\\"
backgroundPosition=\\"center\\"
Expand Down Expand Up @@ -111,7 +111,7 @@ function MyComponent(props) {
</div>
</div>
</Column>
<Column width={}>
<Column>
<Image
image=\\"https://cdn.builder.io/api/v1/image/assets%2FagZ9n5CUKRfbL9t6CaJOyVSK4Es2%2F70c33c597e9946e9a79ab99ad9a999d3?width=998&height=1000\\"
backgroundPosition=\\"center\\"
Expand Down Expand Up @@ -211,7 +211,7 @@ function MyComponent(props) {
</div>
</div>
</Column>
<Column width={}>
<Column>
<Image
image=\\"https://cdn.builder.io/api/v1/image/assets%2FagZ9n5CUKRfbL9t6CaJOyVSK4Es2%2F226d855cb0ee46e4a2c7baa6990c1118?width=998&height=1000\\"
backgroundPosition=\\"center\\"
Expand Down
38 changes: 38 additions & 0 deletions packages/core/src/__tests__/builder.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,44 @@ describe('Builder', () => {
expect(mitosis.trim()).toEqual(code.trim());
});

test('do not generate empty expression for width on Column', () => {
const content = {
data: {
blocks: [
{
'@type': '@builder.io/sdk:Element' as const,
component: {
name: 'Columns',
options: {
columns: [{ blocks: [] }, { blocks: [], width: 50 }],
},
},
},
],
},
};

const mitosisJson = builderContentToMitosisComponent(content);

const mitosis = componentToMitosis(mitosisOptions)({
component: mitosisJson,
});

expect(mitosis).toMatchInlineSnapshot(`
"import { Columns, Column } from \\"@components\\";

export default function MyComponent(props) {
return (
<Columns>
<Column />
<Column width={50} />
</Columns>
);
}
"
`);
});

test('nodes as props', () => {
const content = {
data: {
Expand Down
12 changes: 9 additions & 3 deletions packages/core/src/parsers/builder/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,15 @@ const componentMappers: {
block.component?.options.columns?.map((col: any, index: number) =>
createMitosisNode({
name: 'Column',
bindings: {
width: { code: col.width?.toString() },
},
/**
* If width if undefined, do not create a binding otherwise its JSX will
* be <Column width={} /> which is not valid due to the empty expression.
*/
...(col.width !== undefined && {
bindings: {
width: { code: col.width.toString() },
},
}),
Comment on lines +354 to +358
Copy link

Choose a reason for hiding this comment

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

is it possible to move the undefined check to the expression for width inside the bindings?

Suggested change
...(col.width !== undefined && {
bindings: {
width: { code: col.width.toString() },
},
}),
bindings: {
...(col.width !== undefined && { width: { code: col.width.toString() } }),
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be possible, but I think that would result in an empty bindings: {} if the width is defined. I tried to follow the style below that where don't add the properties object if col.link is undefined

...(col.link && {
properties: {
link: col.link,
Expand Down