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

Sandboxed code examples #649

Merged
merged 30 commits into from
Jan 17, 2019
Merged

Sandboxed code examples #649

merged 30 commits into from
Jan 17, 2019

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Jan 15, 2019

👀 Preview

This wraps all of our rendered code examples in <iframe> elements with the magic of React portals! The trickiest part (by far) was getting the stylesheets into the frames, and required some refactoring of our Next config and a hack to pass an SSR-only file listing down to client-rendered components.

In this PR I'm introducing two new components that both the document template and our sandboxex code examples can use: <CommonStyles /> and <CommonScripts />. The idea is that both of these will render the requisite <link> and <script> tags for both the Primer CSS build and any style guide-specific CSS and JS bundles that we need from dot-com (for components that haven't yet made it into Primer CSS). These live in the ever-expanding src/utils.js — which could probably be reorganized because it really deals more with "pages" and "assets".

@shawnbot shawnbot requested a review from emplums January 15, 2019 19:41
docs/src/utils.js Outdated Show resolved Hide resolved
docs/src/CodeExample.js Outdated Show resolved Hide resolved
@shawnbot shawnbot changed the title [WIP] Sandboxed code examples Sandboxed code examples Jan 15, 2019
@shawnbot

This comment has been minimized.

@@ -1,10 +1,8 @@
/* eslint-disable no-console */
const sync = require('./sync')
const cssLoaderConfig = require('@zeit/next-css/css-loader-config')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so for some reason I wasn't able to use the @zeit/next-css "plugin", but their "loader config" works? ¯\_(ツ)_/¯


// only attempt to sync locally and in CI
if (dev && !configured) {
sync({watch: !CI})
}

// in production, we don't need to compile Primer from SCSS; just inline
// the CSS build!
if (!dev) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way things are set up now, we only need the CSS loader (as opposed to Sass) in production. In fact, the Sass loader doesn't work in production because the Primer CSS modules aren't hoisted such that they all live in node_modules. This is a problem that will go away once we collapse the monorepo.

sassLoaderOptions: {
includePaths: [
resolve(__dirname, '../modules'),
resolve(__dirname, 'node_modules')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary to resolve the CSS build in production.

<CommonStyles />
{renderedStyles}
</Head>
<body data-files={JSON.stringify(files)}>
Copy link
Contributor Author

@shawnbot shawnbot Jan 17, 2019

Choose a reason for hiding this comment

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

This is a workaround for the sad fact that Next.js doesn't allow context to be shared between server and client, so this was the only way that I could figure out how to "transmit" the list of files from the Document to other client-side components, namely Frame.

if (!this.node) return null
this.node.style.height = 'max-content'
const {body} = this.node.contentDocument
return body.offsetHeight > DEFAULT_IFRAME_HEIGHT ? body.offsetHeight : body.scrollHeight
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ugly, and doesn't even really do what I want it to. The problem seems to be that iframe elements have a default height of 150px, and styles like min-height: 0 don't have any effect. offsetHeight and scrollHeight differ for some iframes, but I haven't figured out why, exactly.

{children}
<Text fontWeight="bold" as="div" mt={4} mb={2}>
<Text is="div" fontWeight="bold" mt={4} mb={2}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emplums I had this using as="div", but that didn't work. Did Text get overlooked in the is to as refactor?


componentDidMount() {
this.doc = this.node.contentDocument
const files = JSON.parse(document.body.dataset.files || '[]')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where we "read" the files list set by the Document component. It's serialized with JSON.stringify(), so we should be able to parse it as JSON; if it's not set, we default to parsing the string "[]".

@shawnbot shawnbot requested a review from jonrohan January 17, 2019 18:58
@shawnbot
Copy link
Contributor Author

@jonrohan if you have any time today, I'd love your eyes on this so that I can get it into #647. 🙏

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

This code looks fine. 👍

I still wish we could utilize shadow dom instead of iframes. The height thing in particular bugs me. There are some folks building react shadow dom components https://github.com/Wildhoney/ReactShadow we could also roll our own?

@shawnbot
Copy link
Contributor Author

Thanks, @jonrohan. I'm confident that we can solve the iframe height thing more easily than making Shadow DOM work, especially with the requisite polyfill(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants