-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(gatsby-theme-docz): add optional iframe for preview and editor #1305
Conversation
import * as styles from './styles' | ||
|
||
const CODE_FONT = `<style type="text/css">@import url(https://fonts.googleapis.com/css?family=Inconsolata&display=swap);</style>` | ||
const INITIAL_IFRAME_CONTENT = `<!DOCTYPE html><html><head> ${CODE_FONT} <style> \n body { padding: 0; margin: 0; } </style></head><body><div></div></body></html>` |
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.
Would this not counteract the idea behind the iframe?
I would assume that i would need to reset the body styling and load fonts on my own for a component.
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.
Yes, it would ! This felt a bit hacky, I'm not sure I like having the code_font styles and might remove them, because like you said it would go against what we're trying to do.
Resetting the body margin and padding though shouldn't be too problematic I think. It's not necessary for sure but it felt ugly without it 😅
I was trying to make the playground look exactly the same with or without the iframe but it's probably not a good idea.
@@ -79,14 +99,22 @@ export const Playground = ({ code, scope, language }) => { | |||
</button> | |||
</div> | |||
</div> | |||
{showingCode && ( | |||
<Wrapper height={editorHeight}> |
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.
Am i correct in the assumption that this was done to solve this issue #1271 ?
Because now that you are wrapping the component in a iframe the encapsulation of the editor should not be needed any more. Am i missing something? :)
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.
Am i correct in the assumption that this was done to solve this issue #1271 ?
Yep that's exactly why ! Initially wanted to only wrap the preview.
I'm wrapping the preview and the editor each in an iframe, not wrapping the whole Playground with an iframe so it should be necessary if I'm not mistaken.
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 think the issue was that the css from the component inside the preview was styling the code area of the preview. This should not be possible any more after the component preview is wrapped in a iframe.
@rakannimer Is this the correct usage for this feature?
|
@lkuechler yep ! @Dangoo suggested to make it true by default but I'm not sure I'm convinced because this could be considered a breaking change. So to turn it off you would have to do : <Playground useIframe={false}>
<Alert>Some message</Alert>
</Playground> Btw feel free to join the docz slack to join the conversation. |
|
This PR adds an optional iframe around the editor and preview in Playground.
Still haven't decided if it should be opt-in or opt-out.
Any thoughts on that would be appreciated 👍
Similar PR #1304