Update Primer

[sophshep]

I've updated to our new version of Primer (primer-css@7.0.0-2), as listed here: primer/css#230 (comment). No issues! Everything worked as expected 🎉

I'd love to merge this since it updates all the colors and other various improvements we've made to primer in the time since it was last included here!

cc/ @github/design-systems

[broccolini]

If you need primer-states then include the primer-labels package. States were moved into the labels package, and the labels package was added to the product package. If you don't need states then you can delete all these files and references to them.

[sophshep]

👍 ... i don't need them. will delete.

[broccolini]

Thanks for testing this @sophshep 💖 Happy to hear things are working as expected.

Heads up, this version of Primer is a pre-release which I set up for testing before merging in the monorepo pr. It's totally fine to leave the open source site pointing to this, however I will be updating this to a major release once I've merged in the primer monorepo pr (probably today). You can update to the major release when I've published that if you like. Nothing should really change apart from this number and maybe some content on the primer readme, so should be fairly simple to update.

[nayafia]

@broccolini @sophshep hi! 👋 ok to merge this, or do we need to update version?

[sophshep]

do we need to update version?

@nayafia We should update to an even newer version! I'll take care of this this week.

[sophshep]

This is good to go from the design side. All markup has been updated to allow for new changes from the latest version of Primer.

One thing that has me stumped is the failing CI test. It says $gray-300 is an undefined variable, but it's almost definitely not. I tested it here and it compiled and changed visually, so I'm not sure how to troubleshoot.

Any ideas @github/design-systems & @MikeMcQuaid?

[shawnbot]

Hey @sophshep, I think the problem is that Travis is caching your node_modules directory. I honestly don't know how the Travis cache interacts with having that directory committed in git — which is generally not recommended, but might be necessary here because you need to publish the site on Pages? But it's possible that Travis is overwriting the directory with whatever is in its cache before building.

If you need to publish on Pages with npm modules, there are two ways that might work better than committing node_modules:

1. Write some scripts that copy the requisite SCSS files from node_modules/primer-*/**/*.scss to Jekyll's Sass directory (e.g. _sass), and commit that directory to git.
2. Build the site in CI and publish the resulting static files from Travis using a tool like gh-pages.

I'll try blowing away the cache and restarting the build, and if that works we can discuss whether it makes sense to change the publishing strategy.

[shawnbot]

Well, deleting the cache for this branch didn't do it. I cloned the repo locally, ran script/test, and got a completely different error from Travis (formatted for clarity):

Conversion error:
Jekyll::Converters::Scss encountered an error while converting 'assets/css/index.scss':
File to import not found or unreadable: ./lib/variables/typography.scss.
Load paths: <paths> /Users/shawnbot/github/opensource.guide/assets/css on line 2

A bunch of other load paths were listed where I put the <paths> placeholder, and those are added by this line. Adding nested node_modules directories is unnecessary if you're using npm@3 or greater, which uses a flat dependency tree. You can confirm this by making sure that all of the primer-* directories are listed in node_modules, rather than as subdirectories of the individual modules.

Anyway, that's not really the problem. On my end, it looks like primer-support@4.0.7 was installed without a lib/variables directory:

shawnbot@github:~/github/opensource.guide% tree node_modules/primer-support 
node_modules/primer-support
├── CHANGELOG.md
├── LICENSE
├── README.md
├── index.scss
├── lib
│   └── mixins
│       ├── buttons.scss
│       ├── layout.scss
│       ├── misc.scss
│       └── typography.scss
└── package.json

2 directories, 9 files

But, if unpkg is to be believed, this directory was published, and it includes $gray-300. So... no idea. I reinstalled everything with npm install --force, and that did the right thing. But then I got another, completely different error:

Conversion error:
Jekyll::Converters::Scss encountered an error while converting 'assets/css/index.scss':
Undefined variable: "$alt-body-font". on line 106

[bkeepers]

@shawnbot, @sophshep: sorry for the confusion with tests failing. The fix was to git add -f node_modules/primer* (3c955ad) and then remove node_modules from .gitignore (longer explanation for why this was set up like this in the first place in 874a049).

Quora appears to be acting up, which was breaking the tests again, so I ignored that from the link checker.

[shawnbot]

Fantastic, thanks @bkeepers! 🍻

[WojasKrk]

Wojaskrk@gmail.com