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

Deploy with Now, inline CSS #221

Merged
merged 77 commits into from
Aug 22, 2018
Merged

Deploy with Now, inline CSS #221

merged 77 commits into from
Aug 22, 2018

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Aug 21, 2018

This PR builds on #218 with a couple of changes that make it deployable to Now:

  • The build npm script is now called dist (which makes sense, since it builds the dist directory).
  • There is a now.json that configures the project for static deployment out of the docs directory.
  • I've moved all of the Primer CSS "rendering" into our HTML template, eliminating all of the unpkg.com URLs! 🎉

@shawnbot
Copy link
Contributor Author

shawnbot commented Aug 21, 2018

FYI, I originally thought that moving all of the Primer CSS styles into our Styles doc component would work, but that breaks some assumptions about stylesheet application order because Emotion puts all of its generated CSS into the <head>.

Apparently—TIL!—styles in <body> get applied after those in <head>, so background-color rules generated by Emotion would be applied before the Primer CSS rules, even when the class names are in the right order (Primer CSS first, then Emotion) and the selectors have the same precedence. 🙃

@shawnbot
Copy link
Contributor Author

Also, I ran a Lighthouse audit in Chrome Devtools on this to compare with primer.github.io/primer-react. The live site scores 61 on performance, but this one scores 80. 💥

Before image
After image

@jonrohan
Copy link
Member

I was hoping this would mean we wouldn't need to check in the /docs/ folder which would reduce the amount of merge conflicts. Any chance this would work for that?

@shawnbot
Copy link
Contributor Author

@jonrohan I think this gets at least one step closer to that goal. I tried getting this to just run x0 in the deployment container, but there were some issues with ports and HTTPS upgrading, so I switched back to a static deployment.

I think Now just runs npm run build to prepare the container, then npm start in deployment. So if we're interested in continuing to build our docs with x0, we could probably make that work by:

  1. Rename build:docs to just build.
  2. Set up npm start to run serve (which Next.js uses for its static deployments) in production.

I'm totally happy to give this a stab (maybe in a separate PR?), but I'd like to time-box it. @emplums, what do you think?

@shawnbot shawnbot added the ds-reviewed Removes this issue/PR from the first responder list. label Aug 22, 2018
@shawnbot shawnbot changed the base branch from fewer-scrollbars to release-2.0.1-beta August 22, 2018 20:07
@@ -5,6 +5,7 @@
],
"plugins": [
"add-react-displayname",
"transform-object-rest-spread"
"transform-object-rest-spread",
"preval"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this babel plugin to inline the CSS.

)
{examples.map(example => (
<NavLink
className="menu-item no-underline link-gray-dark"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: only menu-item comes from primer-navigation. The other classes here come from primer-utilities.

}

export default Styles
export default withDefaultTheme(Styles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This allows the theme to override the base styles, so long as you either wrap <Styles /> in a ThemeProvider or pass theme={theme} directly.

"build:docs": "x0 build examples --out-dir docs",
"lint": "eslint src examples",
"prepublishOnly": "npm run build",
"start": "x0 dev examples -op 8888",
"start": "x0 dev examples -op ${PORT:-8888}",
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 was an attempt to get the PORT env variable set by Now working with x0. The problem I ran into once I got that working was that x0's HTTP server doesn't seem to like upgrading from HTTP to HTTPS. ☹️

"dist": "NODE_ENV=production rollup -c && npm run build:css",
"predist": "rm -rf dist",
"prepublishOnly": "npm run dist",
"build:css": "primer-module-build --outputDir dist/css src/primer-react.scss",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use primer-module-build without any additional configuration to compile our custom CSS bundle. This generates dist/css/build.css and dist/css/index.js.

This was referenced Aug 22, 2018
@shawnbot shawnbot merged commit ac80e4c into release-2.0.1-beta Aug 22, 2018
@shawnbot shawnbot removed ds-reviewed Removes this issue/PR from the first responder list. status: review needed labels Aug 22, 2018
@shawnbot shawnbot mentioned this pull request Aug 22, 2018
3 tasks
Copy link

@emplums emplums left a comment

Choose a reason for hiding this comment

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

Looks good to me!

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

Successfully merging this pull request may close these issues.

3 participants