-
Notifications
You must be signed in to change notification settings - Fork 22
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
#7670: Use <IsolatedComponent>
for DocumentView
, EphemeralFormContent
, CustomFormComponent
#8200
Changes from all commits
46d2207
708ba2b
0671574
386b02c
6a273f9
360f843
e3018e8
4760ee0
a2c75b9
9760393
df1ec00
a6dc0fe
652339a
c5aa5f6
3d139a2
123b53a
16542a2
fb4d8a4
a349a66
efb53fe
f195c5c
ccb84c2
cc46b6a
98384ca
fee63aa
e29f974
34af2e3
98b0f8e
82b3dd4
3313bc3
1334a91
0f559cb
8b2f5e8
3695681
9d86817
bf236b9
c7aaa29
ee2aa0a
de42f14
9db1a1d
4a01ac6
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 |
---|---|---|
|
@@ -16,7 +16,8 @@ | |
*/ | ||
|
||
import React from "react"; | ||
import { render, screen } from "@testing-library/react"; | ||
import { render } from "@testing-library/react"; | ||
import { screen } from "shadow-dom-testing-library"; | ||
import ImageCropWidget from "@/components/formBuilder/ImageCropWidget"; | ||
import DescriptionField from "@/components/formBuilder/DescriptionField"; | ||
import JsonSchemaForm from "@rjsf/bootstrap-4"; | ||
|
@@ -26,7 +27,6 @@ import { | |
normalizeIncomingFormData, | ||
normalizeOutgoingFormData, | ||
} from "./customForm"; | ||
import { waitForEffect } from "@/testUtils/testHelpers"; | ||
import userEvent from "@testing-library/user-event"; | ||
|
||
import { dataStore } from "@/background/messenger/strict/api"; | ||
|
@@ -38,6 +38,16 @@ import { toExpression } from "@/utils/expressionUtils"; | |
const dataStoreGetMock = jest.mocked(dataStore.get); | ||
const dataStoreSetSpy = jest.spyOn(dataStore, "set"); | ||
|
||
// I couldn't get shadow-dom-testing-library working | ||
jest.mock("react-shadow/emotion", () => ({ | ||
__esModule: true, | ||
default: { | ||
div(props: any) { | ||
return <div {...props}></div>; | ||
}, | ||
}, | ||
})); | ||
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. 🙌 Good choice. We should probably move it to However I hope that the shadow DOM isn't actually what was breaking this. 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. userEvent was adding the value to the input, but the value wasn't submitted. We've had this issue elsewhere, we probably will end up moving it to |
||
|
||
describe("form data normalization", () => { | ||
const normalizationTestCases = [ | ||
{ | ||
|
@@ -186,7 +196,7 @@ describe("form data normalization", () => { | |
/>, | ||
); | ||
|
||
await waitForEffect(); | ||
await expect(screen.findByRole("button")).resolves.toBeInTheDocument(); | ||
|
||
// Make sure the form renders the data without errors | ||
expect(asFragment()).toMatchSnapshot(); | ||
|
@@ -230,7 +240,7 @@ describe("CustomFormRenderer", () => { | |
render(<Component {...props} />); | ||
|
||
expect(screen.queryByText("Submit")).not.toBeInTheDocument(); | ||
expect(screen.getByRole("textbox")).toBeInTheDocument(); | ||
await expect(screen.findByRole("textbox")).resolves.toBeInTheDocument(); | ||
}); | ||
|
||
test("Supports postSubmitAction reset", async () => { | ||
|
@@ -261,7 +271,7 @@ describe("CustomFormRenderer", () => { | |
|
||
render(<Component {...props} />); | ||
|
||
const textBox = screen.getByRole("textbox"); | ||
const textBox = await screen.findByRole("textbox"); | ||
await userEvent.type(textBox, "Some text"); | ||
expect(textBox).toHaveValue("Some text"); | ||
await userEvent.click(screen.getByRole("button", { name: "Submit" })); | ||
|
@@ -305,8 +315,10 @@ describe("CustomFormRenderer", () => { | |
render(<Component {...props} />); | ||
|
||
const value = "Some text"; | ||
const textBox = screen.getByRole("textbox"); | ||
const textBox = await screen.findByRole("textbox"); | ||
|
||
await userEvent.type(textBox, value); | ||
|
||
await userEvent.click(screen.getByRole("button", { name: "Submit" })); | ||
|
||
expect(runPipelineMock).toHaveBeenCalledOnce(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
import React from "react"; | ||
import { type JsonObject } from "type-fest"; | ||
import { dataStore } from "@/background/messenger/strict/api"; | ||
import { validateRegistryId } from "@/types/helpers"; | ||
|
@@ -46,6 +47,8 @@ import { getOutputReference, validateOutputKey } from "@/runtime/runtimeTypes"; | |
import { type BrickConfig } from "@/bricks/types"; | ||
import { isExpression } from "@/utils/expressionUtils"; | ||
import { getPlatform } from "@/platform/platformContext"; | ||
import IsolatedComponent from "@/components/IsolatedComponent"; | ||
import { type CustomFormComponentProps } from "./CustomFormComponent"; | ||
|
||
interface DatabaseResult { | ||
success: boolean; | ||
|
@@ -297,10 +300,20 @@ export class CustomFormRenderer extends RendererABC { | |
normalizedData, | ||
}); | ||
|
||
// Changed webpackChunkName to de-conflict with the manual entry in webpack used to load in the stylesheets | ||
const { default: CustomFormComponent } = await import( | ||
/* webpackChunkName: "CustomFormRendererComponent" */ | ||
"./CustomFormComponent" | ||
const CustomFormComponent: React.FunctionComponent< | ||
CustomFormComponentProps | ||
> = (props) => ( | ||
<IsolatedComponent | ||
name="CustomFormComponent" | ||
noStyle={disableParentStyles} | ||
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. |
||
lazy={async () => | ||
import( | ||
/* webpackChunkName: "isolated/CustomFormComponent" */ | ||
"./CustomFormComponent" | ||
) | ||
} | ||
factory={(CustomFormComponent) => <CustomFormComponent {...props} />} | ||
/> | ||
); | ||
|
||
return { | ||
|
@@ -314,7 +327,6 @@ export class CustomFormRenderer extends RendererABC { | |
submitCaption, | ||
className, | ||
stylesheets, | ||
disableParentStyles, | ||
// Option only applies if a custom onSubmit handler is provided | ||
resetOnSubmit: onSubmit != null && postSubmitAction === "reset", | ||
async onSubmit( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,8 @@ | |
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
import React from "react"; | ||
import { RendererABC } from "@/types/bricks/rendererTypes"; | ||
import DocumentViewLazy from "./documentView/DocumentViewLazy"; | ||
import { validateRegistryId } from "@/types/helpers"; | ||
import { | ||
type BrickArgs, | ||
|
@@ -28,6 +28,8 @@ import { | |
DOCUMENT_ELEMENT_TYPES, | ||
type DocumentElement, | ||
} from "@/components/documentBuilder/documentBuilderTypes"; | ||
import IsolatedComponent from "@/components/IsolatedComponent"; | ||
import { type DocumentViewProps } from "./documentView/DocumentViewProps"; | ||
|
||
export const DOCUMENT_SCHEMA: Schema = { | ||
$schema: "https://json-schema.org/draft/2019-09/schema#", | ||
|
@@ -107,12 +109,30 @@ export class DocumentRenderer extends RendererABC { | |
}>, | ||
options: BrickOptions, | ||
): Promise<ComponentRef> { | ||
const DocumentView: React.FC<DocumentViewProps> = (props) => ( | ||
<IsolatedComponent | ||
name="DocumentView" | ||
noStyle={disableParentStyles} | ||
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. |
||
lazy={async () => | ||
import( | ||
/* webpackChunkName: "isolated/DocumentView" */ | ||
"./documentView/DocumentView" | ||
) | ||
} | ||
factory={(DocumentView) => <DocumentView {...props} />} | ||
// It must fill the frame even if `noStyle` is set, so set it as a style prop | ||
// TODO: The parent node should instead make sure that the children fill | ||
// the sidebar vertically (via a simple `.d-flex`), but this this requires | ||
// verifying that other components aren't broken by this. | ||
style={{ height: "100%" }} | ||
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. "h-100" restored. 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. 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. Nevermind, that restores the classname, but as you point out, it's not getting the stylesheet at that level. |
||
/> | ||
); | ||
|
||
return { | ||
Component: DocumentViewLazy, | ||
Component: DocumentView, | ||
props: { | ||
body, | ||
stylesheets, | ||
disableParentStyles, | ||
options, | ||
}, | ||
}; | ||
|
This file was deleted.
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.
disableParentStyles
removed from 2 components because now it's not the Component's responsibility to load its own stylesheets, but it's the loader's (i.e. whereIsolatedComponent
is called)