-
Notifications
You must be signed in to change notification settings - Fork 1k
fix[qwik]: ENG-7299 Default value not updating for custom components on ContentEditor #3947
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
Changes from 14 commits
efe828f
76040a7
2c2db77
3c65c71
72ae3ce
02fc383
6d3fc1e
9a5d339
9da6401
d4a35e7
12db0f9
3e92a26
a4310cf
6b3dd9e
2808573
c97f374
ac7f88e
7474198
d8ea63b
3b64cc3
0ba8ceb
3478c53
f780509
126bcfc
68a7c14
7bb3f45
27925d0
6517f39
e133b60
5b585c4
57c5eae
321641b
7620d48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||
import { expect } from '@playwright/test'; | ||||||||||||||||||||||||||
Check failure on line 1 in packages/sdks-tests/src/e2e-tests/editing.spec.ts
|
||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||
COLUMNS_WITH_NEW_SPACE, | ||||||||||||||||||||||||||
COLUMNS_WITH_NEW_TEXT, | ||||||||||||||||||||||||||
|
@@ -18,6 +18,7 @@ | |||||||||||||||||||||||||
import { ADD_A_TEXT_BLOCK } from '../specs/duplicated-content-using-nested-symbols.js'; | ||||||||||||||||||||||||||
import { EDITING_STYLES } from '../specs/editing-styles.js'; | ||||||||||||||||||||||||||
import { ACCORDION_WITH_NO_DETAIL } from '../specs/accordion.js'; | ||||||||||||||||||||||||||
import { CUSTOM_COMPONENT_NO_DEFAULT_VALUE } from '../specs/custom-component-no-default-value.js'; | ||||||||||||||||||||||||||
import { NEW_BLOCK_ADD, NEW_BLOCK_ADD_2 } from '../specs/new-block-add.js'; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const editorTests = ({ | ||||||||||||||||||||||||||
|
@@ -151,6 +152,16 @@ | |||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
test('correctly updating custom components when default value is not set', async ({ page, basePort, sdk, packageName }) => { | ||||||||||||||||||||||||||
test.skip(!['react', 'qwik-city'].includes(packageName)); | ||||||||||||||||||||||||||
await launchEmbedderAndWaitForSdk({ path: '/custom-components-no-default-value', basePort, page, sdk }); | ||||||||||||||||||||||||||
const newContent = cloneContent(CUSTOM_COMPONENT_NO_DEFAULT_VALUE); | ||||||||||||||||||||||||||
newContent.data.blocks[0].component.options.text = "Hello"; | ||||||||||||||||||||||||||
await sendContentUpdateMessage({ page, newContent, model: 'page' }); | ||||||||||||||||||||||||||
const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]') | ||||||||||||||||||||||||||
await expect(helloWorldText).toHaveText('Hello'); | ||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by default it should render There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't get you...🤔 component: {
name: 'Description',
options: {},
} we will be requiring sendContentUpdateMessage to update the text message from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this: https://github.com/BuilderIO/builder/pull/3947/files#diff-f17fc2e9e6719ba501c3ab4df9c1fef742dc2c5dd3f709c622f6c79cdffcc1bdR99 add the defaultValue? i'm confused because on initial render if there is no initial value provided and we are providing a default value - we should be fallbacking to that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sidmohanty11 your confusion stems from the fact that default values are only used when the block is created in the visual editor to determine the initial value for inputs: they are not used by the SDK at run-time. @yash-builder one way to clear up such confusion is to avoid using the exact same values for different things. Have a Also, as a general rule, when we add a test that involves editing content, i think it's best that it looks like this:
In this test you're not checking the initial state of the block. You could add a test.skip(!['react', 'qwik-city'].includes(packageName));
await launchEmbedderAndWaitForSdk({ path: '/custom-components-no-default-value', basePort, page, sdk });
// check initial value here
const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]')
await expect(helloWorldText).toBeEmpty();
const newContent = cloneContent(CUSTOM_COMPONENT_NO_DEFAULT_VALUE);
newContent.data.blocks[0].component.options.text = "Hello";
await sendContentUpdateMessage({ page, newContent, model: 'page' });
const helloWorldText = page.frameLocator('iframe').locator('[builder-id="builder-d01784d1ca964e0da958ab4a4a891b08"]')
await expect(helloWorldText).toHaveText('Hello'); Not a huge deal though, more of a general rule of thumb for these types of tests |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
test('removal of styles should work properly', async ({ page, packageName, sdk, basePort }) => { | ||||||||||||||||||||||||||
test.skip(packageName === 'nextjs-sdk-next-app' || checkIsGen1React(sdk)); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
@@ -332,14 +343,20 @@ | |||||||||||||||||||||||||
model: 'page', | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
await item1.click(); | ||||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||||
page.frameLocator('iframe').getByText('Item 1') | ||||||||||||||||||||||||||
).toBeVisible({ timeout: 5000 }); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Re-query the item1 element as it might have been recreated | ||||||||||||||||||||||||||
const updatedItem1 = page.frameLocator('iframe').getByText('Item 1'); | ||||||||||||||||||||||||||
await updatedItem1.click(); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we want to avoid hardcoded timeouts unless there's a special reason for them. Playwright has automated waiting and retrying so that we don't need to provide hardcoded timeout values all the time. Also, in this case, the default is already 5000 so this does nothing:
you also don't need to redefine the locator twice:
Suggested change
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const detailElement = page.frameLocator('iframe').getByText(NEW_DETAILS_TEXT); | ||||||||||||||||||||||||||
await detailElement.waitFor(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const [titleBox, detailBox] = await Promise.all([ | ||||||||||||||||||||||||||
item1.boundingBox(), | ||||||||||||||||||||||||||
updatedItem1.boundingBox(), | ||||||||||||||||||||||||||
detailElement.boundingBox(), | ||||||||||||||||||||||||||
Check failure on line 359 in packages/sdks-tests/src/e2e-tests/editing.spec.ts
|
||||||||||||||||||||||||||
]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (!titleBox || !detailBox) { | ||||||||||||||||||||||||||
|
@@ -549,7 +566,7 @@ | |||||||||||||||||||||||||
expect(endTextBlockBox).toBeDefined(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (!middleTextBlockBox || !topTextBlockBox || !endTextBlockBox) { | ||||||||||||||||||||||||||
throw new Error('New text block or text block not found'); | ||||||||||||||||||||||||||
Check failure on line 569 in packages/sdks-tests/src/e2e-tests/editing.spec.ts
|
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
expect(middleTextBlockBox.y).toBeGreaterThan(topTextBlockBox.y); | ||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
export const CUSTOM_COMPONENT_NO_DEFAULT_VALUE = { | ||
data: { | ||
title: 'dynamic-button-sdk-test', | ||
themeId: false, | ||
blocks: [ | ||
{ | ||
'@type': '@builder.io/sdk:Element', | ||
'@version': 2, | ||
id: 'builder-d01784d1ca964e0da958ab4a4a891b08', | ||
component: { | ||
name: 'Description', | ||
options: {}, | ||
}, | ||
responsiveStyles: { | ||
large: { | ||
display: 'flex', | ||
flexDirection: 'column', | ||
position: 'relative', | ||
flexShrink: '0', | ||
boxSizing: 'border-box', | ||
marginTop: '20px', | ||
}, | ||
}, | ||
}, | ||
], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { component$ } from '@builder.io/qwik'; | ||
|
||
export const Description = component$((props: { text: string }) => { | ||
return ( | ||
<div> | ||
<h3>{props.text}</h3> | ||
</div> | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { component$ } from '@builder.io/qwik'; | ||
|
||
export const Hello = component$(() => { | ||
return ( | ||
<div>hello</div> | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,6 +3,8 @@ import { routeLoader$ } from '@builder.io/qwik-city'; | |||||
import { Content, _processContentResult } from '@builder.io/sdk-qwik'; | ||||||
import { getProps } from '@sdk/tests'; | ||||||
import BuilderBlockWithClassName from '~/components/BuilderBlockWithClassName'; | ||||||
import { Description } from '~/components/Description'; | ||||||
import { Hello } from '~/components/Hello'; | ||||||
|
||||||
const builderBlockWithClassNameCustomComponent = { | ||||||
name: 'BuilderBlockWithClassName', | ||||||
|
@@ -46,6 +48,25 @@ const builderBlockWithClassNameCustomComponent = { | |||||
], | ||||||
}; | ||||||
|
||||||
const CUSTOM_COMPONENT = [ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
{ | ||||||
name: 'Hello', | ||||||
component: Hello, | ||||||
inputs: [], | ||||||
}, | ||||||
Comment on lines
+53
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you added the Hello component for this test:https://github.com/sidmohanty11/builder/blob/general-support-vc/packages/sdks-tests/src/e2e-tests/custom-components.spec.ts#L11 right? you will also need to update the content of it from just hello to |
||||||
{ | ||||||
name: 'Description', | ||||||
component: Description, | ||||||
inputs: [ | ||||||
{ | ||||||
name: 'text', | ||||||
type: 'string', | ||||||
defaultValue: 'Hello', | ||||||
}, | ||||||
], | ||||||
}, | ||||||
]; | ||||||
|
||||||
export const useBuilderContentLoader = routeLoader$(async (event) => { | ||||||
const data = await getProps({ | ||||||
pathname: event.url.pathname, | ||||||
|
@@ -67,7 +88,9 @@ export default component$(() => { | |||||
{contentProps.value ? ( | ||||||
<Content | ||||||
{...(contentProps.value as any)} | ||||||
customComponents={[builderBlockWithClassNameCustomComponent]} | ||||||
customComponents={[ | ||||||
...CUSTOM_COMPONENT, | ||||||
builderBlockWithClassNameCustomComponent]} | ||||||
/> | ||||||
) : ( | ||||||
<div>Content Not Found</div> | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export const Description = (props: { text: string }) => { | ||
return ( | ||
<div> | ||
<h3>{props.text}</h3> | ||
</div> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -960,6 +960,21 @@ const QWIK_ONUPDATE_TO_USEVISIBLETASK = () => ({ | |||
}, | ||||
}); | ||||
|
||||
const QWIK_FORCE_RENDER_COUNT_FOR_RENDERING_CUSTOM_COMPONENT_DEFAULT_VALUE = () => ({ | ||||
json: { | ||||
post: (json) => { | ||||
if (json.name === 'InteractiveElement') { | ||||
console.log('json', JSON.stringify(json, null, 2)); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
json.children[0].meta.else.bindings['key'] = { | ||||
code: "'wrapper-' + state.forceRenderCount", | ||||
bindingType: "expression", | ||||
type: "single" | ||||
}; | ||||
} | ||||
}, | ||||
}, | ||||
}); | ||||
|
||||
/** | ||||
* @type {MitosisConfig} | ||||
*/ | ||||
|
@@ -1239,6 +1254,7 @@ module.exports = { | |||
}, | ||||
}, | ||||
}), | ||||
QWIK_FORCE_RENDER_COUNT_FOR_RENDERING_CUSTOM_COMPONENT_DEFAULT_VALUE, | ||||
QWIK_ONUPDATE_TO_USEVISIBLETASK, | ||||
], | ||||
}, | ||||
|
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.
this line doesn't have any impact,
sdk
can beqwik
.packageName
s are the different e2e/snippet server names and can beqwik-city
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.
Ahh nice catch....I was testing somethings...I forgot to remove this