-
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
Conversation
…nto F/feature/isolated-form-and-document
…F/feature/isolated-form-and-document
To avoid this regression: #8211
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 seems to work, except customForm.test
is failing and I'm not sure why
> = (props) => ( | ||
<IsolatedComponent | ||
name="CustomFormComponent" | ||
noStyle={disableParentStyles} |
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.
const DocumentView: React.FC<DocumentViewProps> = (props) => ( | ||
<IsolatedComponent | ||
name="DocumentView" | ||
noStyle={disableParentStyles} |
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.
> = (props) => ( | ||
<IsolatedComponent | ||
name="EphemeralFormContent" | ||
noStyle={props.definition.disableParentStyles} |
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.
|
||
expect(screen.queryByText("Submit")).not.toBeInTheDocument(); | ||
expect(screen.getByRole("textbox")).toBeInTheDocument(); | ||
expect(screen.getByShadowRole("textbox")).toBeInTheDocument(); |
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.
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.
@@ -228,9 +229,10 @@ describe("CustomFormRenderer", () => { | |||
); | |||
|
|||
render(<Component {...props} />); | |||
await waitForEffect(); |
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.
Appears to be needed due to the new React.lazy/Suspense usage.
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.
await screen.findByRole("textbox"0
or similar usually works instead of await waitForEffect()
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(); |
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.
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
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.
I don't have time today, but I may have time tomorrow
@@ -63,8 +65,11 @@ const CustomFormComponent: React.FunctionComponent<{ | |||
resetOnSubmit?: boolean; | |||
className?: string; | |||
stylesheets?: string[]; | |||
disableParentStyles?: boolean; |
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. where IsolatedComponent
is called)
@@ -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", |
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.
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.
src/components/IsolatedComponent.tsx
Outdated
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.
Changes to this file just come from:
Looking into this now |
// 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 comment
The 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 comment
The 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 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.
Should be good to go now |
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. |
return <div {...props}></div>; | ||
}, | ||
}, | ||
})); |
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.
🙌
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.
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.
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.
Feel free to merge when ready |
These are the main 3 components that caused trouble with the stylesheet loads, so this PR should finally fix it.
Checklist
h-100
onDocumentView
disableParentStyles
still works in all 3What does this PR do?
IsolatedComponent
) #8151Discussion
It looks like every renderer has its ownEmotionShadowRoot
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 nestedCustomForm
Future Work