-
Notifications
You must be signed in to change notification settings - Fork 0
chore: Adds iframe helper and iframe vr test #61
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
Conversation
pages/utils/iframe-wrapper.tsx
Outdated
if (!container) { | ||
return; | ||
} | ||
const iframeEl = container.ownerDocument.createElement("iframe"); |
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.
Could the iframe be part of the returned component JSX instead? (even if we then imperatively modify its content, maybe we can at least avoid imperatively creating it)
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 should work. The original code was copied from here: https://github.com/cloudscape-design/components/blob/main/pages/utils/iframe-wrapper.tsx#L39, but I think we can make an improvement and then decide on which approach is better if creating a reusable util in the toolkit.
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.
Interestingly, the code that creates and removes iframe was deliberate: cloudscape-design/components#2499
However, I checked the current version and it does work in Firefox.
|
||
const innerAppRoot = iframeDocument.createElement("div"); | ||
iframeDocument.body.appendChild(innerAppRoot); | ||
copyStyles(document, iframeDocument); |
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.
Do we expect our consumers to copy their styles in a similar way?
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.
No, but the iframe should have all necessary styles bundled.
Description
Rel: [ZaftArmITulj]
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.