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

Colocate Primer CSS global injections with components that need them #285

Merged
merged 20 commits into from
Sep 26, 2018

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Sep 26, 2018

This is a follow-up to #276 that breaks up our global CSS injections so that we can more easily remove them as we port components over to Emotion. Here's why I think we should do this:

  1. It can make user bundles smaller if they take advantage of tree-shaking because they'll only get the CSS for components that they use.
  2. It should make refactoring a component with Emotion simpler, because it makes it really clear where we should look for the original component CSS, and we can more easily test removing the globally injected styles by commenting out a single statement in the component we're working on.

I clicked through all of the component docs to make sure that I didn't break anything, and fleshed out a couple of documentation bits and examples while I was in there.

Details changes

The Details component is kind of a weird case because the details-reset CSS lives in the primer-buttons module. Rather than double up Sass imports or inject them globally, I opted to make a styled('details') with the details-reset styles and use that in the Details component. The Details docs also now mention that you have bring your own <summary> (or <Button is="summary">), and make it clearer that if you use the children function or render prop you have to implement the toggle function yourself.

Merge checklist

  • Changed base branch to release branch
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@vercel
Copy link

vercel bot commented Sep 26, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

import sass from 'sass.macro'
import {injectGlobal} from 'emotion'

injectGlobal(sass`
Copy link
Contributor Author

@shawnbot shawnbot Sep 26, 2018

Choose a reason for hiding this comment

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

Even though we're not using any actual Sass features here, processing it with Sass allows us to minify it with outputStyle: 'compressed'.

@shawnbot shawnbot changed the title [WIP] Colocate Primer CSS global injections with components that need them Colocate Primer CSS global injections with components that need them Sep 26, 2018
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!

@shawnbot shawnbot merged commit 09fe10c into q3-wildflower Sep 26, 2018
@shawnbot shawnbot deleted the colocate-css branch September 26, 2018 23:13
@emplums emplums mentioned this pull request Sep 28, 2018
6 tasks
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.

2 participants