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

#7670: Use <IsolatedComponent> for DocumentView, EphemeralFormContent, CustomFormComponent #8200

Merged
merged 41 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
46d2207
POC: Isolated components
fregante Apr 3, 2024
708ba2b
Wrap `SelectionMenu`
fregante Apr 3, 2024
0671574
Extract CSS, use regular element for now
fregante Apr 3, 2024
386b02c
Wrap `PropertyTree`
fregante Apr 3, 2024
6a273f9
Wrap `SelectionToolPopover`
fregante Apr 3, 2024
360f843
Fix tests
fregante Apr 3, 2024
e3018e8
Fix unrelated mutation
fregante Apr 3, 2024
4760ee0
Update docs
fregante Apr 3, 2024
a2c75b9
Cleanup
fregante Apr 3, 2024
9760393
Some components need no style; test Stylesheets component
fregante Apr 4, 2024
df1ec00
Ok but no fonts?
fregante Apr 6, 2024
a6dc0fe
Ok with fonts
fregante Apr 6, 2024
652339a
Import issues
fregante Apr 6, 2024
c5aa5f6
Lint
fregante Apr 6, 2024
3d139a2
Move init to <IsolatedComponent>
fregante Apr 6, 2024
123b53a
Fix most issues
fregante Apr 6, 2024
16542a2
Merge branch 'main' into F/feature/isolated-components
fregante Apr 6, 2024
fb4d8a4
Extract/cleanup
fregante Apr 6, 2024
a349a66
/2
fregante Apr 6, 2024
efb53fe
Move DiscardFilePlugin config out
fregante Apr 6, 2024
f195c5c
Update documentation
fregante Apr 8, 2024
ccb84c2
CustomFormComponent
fregante Apr 8, 2024
cc46b6a
EphemeralFormContent
fregante Apr 8, 2024
98384ca
DocumentView
fregante Apr 8, 2024
fee63aa
Merge remote-tracking branch 'origin/main' into F/feature/isolated-co…
fregante Apr 8, 2024
e29f974
Bad merge
fregante Apr 8, 2024
34af2e3
Merge remote-tracking branch 'origin/F/feature/isolated-components' i…
fregante Apr 8, 2024
98b0f8e
Lint
fregante Apr 8, 2024
82b3dd4
Lint
fregante Apr 8, 2024
3313bc3
Merge remote-tracking branch 'origin/main' into F/feature/isolated-co…
fregante Apr 10, 2024
1334a91
Sort jest config
fregante Apr 10, 2024
0f559cb
Merge remote-tracking branch 'origin/F/feature/isolated-components' i…
fregante Apr 10, 2024
8b2f5e8
Merge remote-tracking branch 'origin/main' into F/feature/isolated-fo…
fregante Apr 10, 2024
3695681
Ensure stylesheets are removed in IsolatedComponent
fregante Apr 10, 2024
9d86817
Comments
fregante Apr 10, 2024
bf236b9
Merge remote-tracking branch 'origin/F/bug/isolated-components' into …
fregante Apr 10, 2024
c7aaa29
Open Shadow DOM
fregante Apr 10, 2024
ee2aa0a
Test shadow DOM
fregante Apr 10, 2024
de42f14
Merge remote-tracking branch 'origin/main' into F/feature/isolated-fo…
fregante Apr 11, 2024
9db1a1d
Fill the frame (h-100)
fregante Apr 11, 2024
4a01ac6
fixes failing tests
grahamlangford Apr 11, 2024
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: 0 additions & 5 deletions scripts/DiscardFilePlugin.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,6 @@ export default class DiscardFilePlugin {
// If `delete assets[]` causes issues in the future, try replacing the content instead:
// assets["DocumentView.js"] = new webpack.sources.RawSource('"Dropped"');
}

// TODO: Use <IsolatedComponent/> and move these to isolatedComponentList
delete assets["DocumentView.js"];
delete assets["EphemeralFormContent.js"];
delete assets["CustomFormComponent.js"];
},
);
});
Expand Down
23 changes: 11 additions & 12 deletions src/bricks/renderers/CustomFormComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import "@/vendors/bootstrapWithoutRem.css";
import "@/sidebar/sidebarBootstrapOverrides.scss";
import "@/bricks/renderers/customForm.css";
import React, { useEffect, useRef, useState } from "react";
import { type Schema, type UiSchema } from "@/types/schemaTypes";
import { type JsonObject } from "type-fest";
Expand All @@ -24,14 +27,13 @@ import { Stylesheets } from "@/components/Stylesheets";
import JsonSchemaForm from "@rjsf/bootstrap-4";
import validator from "@/validators/formValidator";
import { type IChangeEvent } from "@rjsf/core";
import { templates } from "@/components/formBuilder/RjsfTemplates";
import ImageCropWidget from "@/components/formBuilder/ImageCropWidget";
import RjsfSelectWidget from "@/components/formBuilder/RjsfSelectWidget";
import DescriptionField from "@/components/formBuilder/DescriptionField";
import TextAreaWidget from "@/components/formBuilder/TextAreaWidget";
import RjsfSubmitContext from "@/components/formBuilder/RjsfSubmitContext";
import { templates } from "@/components/formBuilder/RjsfTemplates";
import { cloneDeep } from "lodash";
import { useStylesheetsContextWithFormDefault } from "@/components/StylesheetsContext";

const FIELDS = {
DescriptionField,
Expand All @@ -43,7 +45,7 @@ const UI_WIDGETS = {
TextareaWidget: TextAreaWidget,
} as const;

const CustomFormComponent: React.FunctionComponent<{
export type CustomFormComponentProps = {
schema: Schema;
uiSchema: UiSchema;
submitCaption: string;
Expand All @@ -63,8 +65,11 @@ const CustomFormComponent: React.FunctionComponent<{
resetOnSubmit?: boolean;
className?: string;
stylesheets?: string[];
disableParentStyles?: boolean;
Copy link
Contributor Author

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. where IsolatedComponent is called)

}> = ({
};

const CustomFormComponent: React.FunctionComponent<
CustomFormComponentProps
> = ({
schema,
uiSchema,
submitCaption,
Expand All @@ -73,8 +78,7 @@ const CustomFormComponent: React.FunctionComponent<{
className,
onSubmit,
resetOnSubmit = false,
stylesheets: newStylesheets,
disableParentStyles = false,
stylesheets,
}) => {
// Use useRef instead of useState because we don't need/want a re-render when count changes
// This ref is used to track the onSubmit run number for runtime tracing
Expand All @@ -95,11 +99,6 @@ const CustomFormComponent: React.FunctionComponent<{
setKey((prev) => prev + 1);
};

const { stylesheets } = useStylesheetsContextWithFormDefault({
newStylesheets,
disableParentStyles,
});

const submitData = async (data: UnknownObject): Promise<void> => {
submissionCountRef.current += 1;
await onSubmit(data, {
Expand Down
24 changes: 16 additions & 8 deletions src/bricks/renderers/customForm.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -228,9 +229,10 @@
);

render(<Component {...props} />);
await waitForEffect();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appears to be needed due to the new React.lazy/Suspense usage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

await screen.findByRole("textbox"0 or similar usually works instead of await waitForEffect()


expect(screen.queryByText("Submit")).not.toBeInTheDocument();
expect(screen.getByRole("textbox")).toBeInTheDocument();
expect(screen.getByShadowRole("textbox")).toBeInTheDocument();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure why suddenly this is required, the component was already in a shadow DOM.

For ease of use I tried mocking the EmotionShadowRoot component but that did not remove the shadow root. 🤷‍♂️ The mode is already set to open in tests too

Anyway this part now works.

});

test("Supports postSubmitAction reset", async () => {
Expand Down Expand Up @@ -261,17 +263,19 @@

render(<Component {...props} />);

const textBox = screen.getByRole("textbox");
await waitForEffect();

const textBox = screen.getByShadowRole("textbox");
await userEvent.type(textBox, "Some text");
expect(textBox).toHaveValue("Some text");
await userEvent.click(screen.getByRole("button", { name: "Submit" }));
await userEvent.click(screen.getByShadowRole("button", { name: "Submit" }));

expect(runPipelineMock).toHaveBeenCalledOnce();

expect(dataStoreSetSpy).not.toHaveBeenCalled();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of these call assertions are failing. Can anyone look into them? The component is rendered fine, I see SET_PAGE_STATE when I click on a Form

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have time today, but I may have time tomorrow


// Need to get new textbox reference, because the old one is removed when the key changes
expect(screen.getByRole("textbox")).toHaveValue("");
expect(screen.getByShadowRole("textbox")).toHaveValue("");
});

test.each([undefined, "save"])(
Expand Down Expand Up @@ -304,18 +308,22 @@

render(<Component {...props} />);

await waitForEffect();

const value = "Some text";
const textBox = screen.getByRole("textbox");
const textBox = screen.getByShadowRole("textbox");
await userEvent.type(textBox, value);
await userEvent.click(screen.getByRole("button", { name: "Submit" }));
await userEvent.click(
screen.getByShadowRole("button", { name: "Submit" }),
);

expect(runPipelineMock).toHaveBeenCalledOnce();
expect(dataStoreSetSpy).toHaveBeenCalledExactlyOnceWith("test", {

Check failure on line 321 in src/bricks/renderers/customForm.test.tsx

View workflow job for this annotation

GitHub Actions / test

CustomFormRenderer › postSubmitAction: undefined doesn't reset

expect(received).toHaveBeenCalledExactlyOnceWith(expected) Expected mock function to have been called exactly once with ["test", {"name": "Some text"}], but it was called with "test" at toHaveBeenCalledExactlyOnceWith (src/bricks/renderers/customForm.test.tsx:321:31)

Check failure on line 321 in src/bricks/renderers/customForm.test.tsx

View workflow job for this annotation

GitHub Actions / test

CustomFormRenderer › postSubmitAction: save doesn't reset

expect(received).toHaveBeenCalledExactlyOnceWith(expected) Expected mock function to have been called exactly once with ["test", {"name": "Some text"}], but it was called with "test" at toHaveBeenCalledExactlyOnceWith (src/bricks/renderers/customForm.test.tsx:321:31)
name: value,
});

// Need to get new textbox reference, because the old one is removed if/when the key changes
expect(screen.getByRole("textbox")).toHaveValue(value);
expect(screen.getByShadowRole("textbox")).toHaveValue(value);
},
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

disableParentStyles verified. See button:

Screenshot 2

lazy={async () =>
import(
/* webpackChunkName: "isolated/CustomFormComponent" */
"./CustomFormComponent"
)
}
factory={(CustomFormComponent) => <CustomFormComponent {...props} />}
/>
);

return {
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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#",
Expand Down Expand Up @@ -107,12 +109,30 @@ export class DocumentRenderer extends RendererABC {
}>,
options: BrickOptions,
): Promise<ComponentRef> {
const DocumentView: React.FC<DocumentViewProps> = (props) => (
<IsolatedComponent
name="DocumentView"
noStyle={disableParentStyles}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

disableParentStyles verified. This is a document with a form:

Screenshot 2

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%" }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"h-100" restored.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I restored it as an actual className:

image

Copy link
Collaborator

Choose a reason for hiding this comment

The 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,
},
};
Expand Down
48 changes: 18 additions & 30 deletions src/bricks/renderers/documentView/DocumentView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,18 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import "@/vendors/bootstrapWithoutRem.css";
import "@/sidebar/sidebarBootstrapOverrides.scss";
import { buildDocumentBranch } from "@/components/documentBuilder/documentTree";
import React from "react";
import EmotionShadowRoot from "@/components/EmotionShadowRoot";
import { type DocumentViewProps } from "./DocumentViewProps";
import DocumentContext from "@/components/documentBuilder/render/DocumentContext";
import { Stylesheets } from "@/components/Stylesheets";
import { joinPathParts } from "@/utils/formUtils";
import StylesheetsContext, {
useStylesheetsContextWithDocumentDefault,
} from "@/components/StylesheetsContext";

const DocumentView: React.FC<DocumentViewProps> = ({
body,
stylesheets: newStylesheets,
disableParentStyles,
stylesheets,
options,
meta,
onAction,
Expand All @@ -44,35 +41,26 @@ const DocumentView: React.FC<DocumentViewProps> = ({
throw new Error("meta.extensionId is required for DocumentView");
}

const { stylesheets } = useStylesheetsContextWithDocumentDefault({
newStylesheets,
disableParentStyles,
});

return (
// Wrap in a React context provider that passes BrickOptions down to any embedded bricks
<DocumentContext.Provider value={{ options, onAction }}>
<EmotionShadowRoot className="h-100">
<StylesheetsContext.Provider value={{ stylesheets }}>
<Stylesheets href={stylesheets}>
{body.map((documentElement, index) => {
const documentBranch = buildDocumentBranch(documentElement, {
staticId: joinPathParts("body", "children"),
// Root of the document, so no branches taken yet
branches: [],
});
<Stylesheets href={stylesheets}>
{body.map((documentElement, index) => {
const documentBranch = buildDocumentBranch(documentElement, {
staticId: joinPathParts("body", "children"),
// Root of the document, so no branches taken yet
branches: [],
});

if (documentBranch == null) {
return null;
}
if (documentBranch == null) {
return null;
}

const { Component, props } = documentBranch;
// eslint-disable-next-line react/no-array-index-key -- They have no other unique identifier
return <Component key={index} {...props} />;
})}
</Stylesheets>
</StylesheetsContext.Provider>
</EmotionShadowRoot>
const { Component, props } = documentBranch;
// eslint-disable-next-line react/no-array-index-key -- They have no other unique identifier
return <Component key={index} {...props} />;
})}
</Stylesheets>
</DocumentContext.Provider>
);
};
Expand Down
35 changes: 0 additions & 35 deletions src/bricks/renderers/documentView/DocumentViewLazy.tsx

This file was deleted.

5 changes: 1 addition & 4 deletions src/bricks/renderers/documentView/DocumentViewProps.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,7 @@ export type DocumentViewProps = {
* Remote stylesheets (URLs) to include in the document.
*/
stylesheets?: string[];
/**
* Whether to disable the base (bootstrap) styles, plus any inherited styles, on the document (and children).
*/
disableParentStyles?: boolean;

options: BrickOptions<BrickArgsContext>;
meta: {
runId: UUID;
Expand Down
Loading
Loading