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

fregante · 2024-04-08T14:26:38Z

These are the main 3 components that caused trouble with the stylesheet loads, so this PR should finally fix it.

Checklist

Restore h-100 on DocumentView
Update tests
Ensure that disableParentStyles still works in all 3
Ensure that this still applies and that it makes sense where it's being loaded:
- #6083: override bootstrap styles in side panel content #6203
Self-review PR
Designate a primary reviewer: @grahamlangford

What does this PR do?

Continues #7670: Simplify creation of isolated widgets (IsolatedComponent) #8151

Discussion

~~It looks like every renderer has its own EmotionShadowRoot and I'm not touching that here, but I think it can/should be dropped if every component is already wrapped.~~

This is an example DOM of a DocumentView and a nested CustomForm

Future Work

Review the ephemeral form frame usage

https://togithub.com/sindresorhus/eslint-plugin-unicorn/issues/1514 https://togithub.com/pixiebrix/pixiebrix-extension/pull/4486

…mponents

…nto F/feature/isolated-form-and-document

…rm-and-document

…F/feature/isolated-form-and-document

To avoid this regression: #8211

fregante

This seems to work, except customForm.test is failing and I'm not sure why

fregante · 2024-04-10T16:29:17Z

src/bricks/renderers/customForm.tsx

+    > = (props) => (
+      <IsolatedComponent
+        name="CustomFormComponent"
+        noStyle={disableParentStyles}


disableParentStyles verified. See button:

fregante · 2024-04-10T16:30:03Z

src/bricks/renderers/document.tsx

+    const DocumentView: React.FC<DocumentViewProps> = (props) => (
+      <IsolatedComponent
+        name="DocumentView"
+        noStyle={disableParentStyles}


disableParentStyles verified. This is a document with a form:

fregante · 2024-04-10T16:30:37Z

src/bricks/transformers/ephemeralForm/EphemeralForm.tsx

+> = (props) => (
+  <IsolatedComponent
+    name="EphemeralFormContent"
+    noStyle={props.definition.disableParentStyles}


props.definition.disableParentStyles verified:

fregante · 2024-04-10T17:04:49Z

src/bricks/renderers/customForm.test.tsx


    expect(screen.queryByText("Submit")).not.toBeInTheDocument();
-    expect(screen.getByRole("textbox")).toBeInTheDocument();
+    expect(screen.getByShadowRole("textbox")).toBeInTheDocument();


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.

fregante · 2024-04-10T17:05:06Z

src/bricks/renderers/customForm.test.tsx

@@ -228,9 +229,10 @@ describe("CustomFormRenderer", () => {
    );

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


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

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

fregante · 2024-04-10T17:06:36Z

src/bricks/renderers/customForm.test.tsx

    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();


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

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

fregante · 2024-04-10T17:09:59Z

src/bricks/renderers/CustomFormComponent.tsx

@@ -63,8 +65,11 @@ const CustomFormComponent: React.FunctionComponent<{
  resetOnSubmit?: boolean;
  className?: string;
  stylesheets?: string[];
-  disableParentStyles?: boolean;


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)

fregante · 2024-04-10T17:10:54Z

webpack.config.mjs

@@ -257,7 +252,7 @@ const createConfig = (env, options) =>
        DEV_EVENT_TELEMETRY: false,
        SANDBOX_LOGGING: false,
        IS_BETA: process.env.PUBLIC_NAME === "-beta",
-        SHADOW_DOM: "closed",
+        SHADOW_DOM: "open",


Defaulting it to open for now. We can review it later. In reality we only need closed on injected widgets (not in the sidebar) so maybe this logic can be moved to renderWidget somehow.

fregante · 2024-04-10T17:12:04Z

src/components/IsolatedComponent.tsx

Changes to this file just come from:

#7670: Ensure stylesheets are removed in IsolatedComponent #8213

…rm-and-document

grahamlangford · 2024-04-11T16:37:36Z

Looking into this now

fregante · 2024-04-11T17:16:15Z

src/bricks/renderers/document.tsx

+        // 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%" }}


"h-100" restored.

Oh, I restored it as an actual className:

Nevermind, that restores the classname, but as you point out, it's not getting the stylesheet at that level.

grahamlangford · 2024-04-11T17:57:50Z

Should be good to go now

github-actions · 2024-04-11T17:59:44Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

fregante · 2024-04-11T18:00:42Z

src/bricks/renderers/customForm.test.tsx

+      return <div {...props}></div>;
+    },
+  },
+}));


🙌

Good choice. We should probably move it to __mocks__ so that it applies to all tests automatically.

However I hope that the shadow DOM isn't actually what was breaking this.

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 __mocks__. We can leverage Playwright to test shadow DOMs in the future.

fregante · 2024-04-11T18:02:32Z

Feel free to merge when ready

fregante added 24 commits April 4, 2024 18:11

POC: Isolated components

46d2207

Wrap SelectionMenu

708ba2b

Extract CSS, use regular element for now

0671574

Wrap PropertyTree

386b02c

Wrap SelectionToolPopover

6a273f9

Fix tests

360f843

Fix unrelated mutation

e3018e8

https://togithub.com/sindresorhus/eslint-plugin-unicorn/issues/1514 https://togithub.com/pixiebrix/pixiebrix-extension/pull/4486

Update docs

4760ee0

Cleanup

a2c75b9

Some components need no style; test Stylesheets component

9760393

Ok but no fonts?

df1ec00

Ok with fonts

a6dc0fe

Import issues

652339a

Lint

c5aa5f6

Move init to <IsolatedComponent>

3d139a2

Fix most issues

123b53a

Merge branch 'main' into F/feature/isolated-components

16542a2

Extract/cleanup

fb4d8a4

/2

a349a66

Move DiscardFilePlugin config out

efb53fe

Update documentation

f195c5c

CustomFormComponent

ccb84c2

EphemeralFormContent

cc46b6a

DocumentView

98384ca

fregante added infrastructure developer experience labels Apr 8, 2024

fregante self-assigned this Apr 8, 2024

fregante added 2 commits April 8, 2024 22:38

Merge remote-tracking branch 'origin/main' into F/feature/isolated-co…

fee63aa

…mponents

Bad merge

e29f974

vercel bot deployed to Preview April 8, 2024 16:14 View deployment

fregante added 2 commits April 10, 2024 22:16

Merge remote-tracking branch 'origin/F/feature/isolated-components' i…

0f559cb

…nto F/feature/isolated-form-and-document

Merge remote-tracking branch 'origin/main' into F/feature/isolated-fo…

8b2f5e8

…rm-and-document

vercel bot deployed to Preview April 10, 2024 15:21 View deployment

fregante added 5 commits April 10, 2024 22:41

Ensure stylesheets are removed in IsolatedComponent

3695681

Comments

9d86817

Merge remote-tracking branch 'origin/F/bug/isolated-components' into …

bf236b9

…F/feature/isolated-form-and-document

Open Shadow DOM

c7aaa29

To avoid this regression: #8211

Test shadow DOM

ee2aa0a

fregante marked this pull request as ready for review April 10, 2024 17:02

vercel bot deployed to Preview April 10, 2024 17:03 View deployment

fregante commented Apr 10, 2024

View reviewed changes

fregante assigned grahamlangford Apr 10, 2024

Merge remote-tracking branch 'origin/main' into F/feature/isolated-fo…

de42f14

…rm-and-document

vercel bot deployed to Preview April 11, 2024 04:34 View deployment

Fill the frame (h-100)

9db1a1d

fregante commented Apr 11, 2024

View reviewed changes

vercel bot deployed to Preview April 11, 2024 17:16 View deployment

fixes failing tests

4a01ac6

grahamlangford approved these changes Apr 11, 2024

View reviewed changes

vercel bot deployed to Preview April 11, 2024 17:59 View deployment

fregante commented Apr 11, 2024

View reviewed changes

grahamlangford merged commit 672f644 into main Apr 11, 2024
25 of 26 checks passed

grahamlangford deleted the F/feature/isolated-form-and-document branch April 11, 2024 18:28

grahamlangford mentioned this pull request Apr 12, 2024

Sidebar and modal styling fixes #8234

Merged

1 task

grahamlangford added this to the 1.8.13 milestone Apr 22, 2024

mnholtz mentioned this pull request Apr 27, 2024

#8347 Fix custom document stylesheets nested stylesheet inheritance #8353

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fregante commented Apr 8, 2024 •

edited

Loading

fregante left a comment

fregante Apr 10, 2024

fregante Apr 10, 2024

fregante Apr 10, 2024

fregante Apr 10, 2024

fregante Apr 10, 2024

grahamlangford Apr 10, 2024

fregante Apr 10, 2024

grahamlangford Apr 10, 2024

fregante Apr 10, 2024

fregante Apr 10, 2024

fregante Apr 10, 2024

grahamlangford commented Apr 11, 2024

fregante Apr 11, 2024

grahamlangford Apr 11, 2024

grahamlangford Apr 11, 2024

grahamlangford commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

fregante Apr 11, 2024

grahamlangford Apr 11, 2024

fregante commented Apr 11, 2024

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

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

Conversation

fregante commented Apr 8, 2024 • edited Loading

Checklist

What does this PR do?

Discussion

Future Work

fregante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamlangford commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamlangford commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Apr 11, 2024

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

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

fregante commented Apr 8, 2024 •

edited

Loading