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

Use style-guide to import fonts and favicons #1582

Merged
merged 1 commit into from
Aug 3, 2017
Merged

Conversation

hursey013
Copy link
Member

This uses the identity-style-guide NPM package to import shared assets. This first PR imports relatively low risk assets: font files, and favicons. A follow up PR will address shared stylesheets, images, and possibly some Javascript.

@hursey013 hursey013 self-assigned this Aug 1, 2017
@hursey013 hursey013 force-pushed the bh-use-style-guide branch from 0999be4 to 29539b3 Compare August 1, 2017 20:14
spec/svg_spec.rb Outdated
@@ -1,7 +1,7 @@
require 'rails_helper'

RSpec.describe 'SVG files' do
Dir[Rails.root.join('**', '*.svg')].each do |svg_path|
Dir[Rails.root.join('**', '*.svg')].reject { |f| f['node_modules'] }.each do |svg_path|
Copy link
Contributor

Choose a reason for hiding this comment

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

is this rejecting files that contain node_modules in the path? I would be more explicit than using the indexing syntax"

.reject { |f| f.include?('node_modules') }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there are some offenders in the identity-style-guide npm package so this is a temporary measure to exclude those for now. None of the images are being used within the app at this point. I will update to the syntax you provided... I wasn't sure the best way to handle excluding just that one folder.

@hursey013 hursey013 force-pushed the bh-use-style-guide branch from 29539b3 to 42277bf Compare August 1, 2017 20:50
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@hursey013 hursey013 force-pushed the bh-use-style-guide branch from 448966c to 03ddda5 Compare August 2, 2017 15:57
Copy link
Contributor

@monfresh monfresh left a comment

Choose a reason for hiding this comment

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

What about the vulnerabilities reported by Snyk? What are we doing about them? Seems like there isn't a fix for the shelljs vulnerability right now. Can we use something else?

@hursey013
Copy link
Member Author

Yeah I'm not sure what to do about this - there are a number of dependencies that don't seem to currently have a fix. All of these dependencies are brought in because the identity-style-guide npm package contains a static site generator called Fractal thats used to generate a style guide on Federalist.

None of these dependencies are actually used within the identity-idp app, I'm just copying asset files out of the package folder into the app. Since they are not being used is it safe to ignore them, or does their presence in the node_modules folder alone cause a potential risk?

@monfresh
Copy link
Contributor

monfresh commented Aug 2, 2017

Hmm. Sounds like the identity-style-guide npm package should perhaps not include all of those dependencies if they're only specific to Fractal?

@monfresh
Copy link
Contributor

monfresh commented Aug 2, 2017

I think that's probably the best solution. If the style guide is meant to be used outside of Federalist, and if fractal is specific to Federalist, then I would remove fractal as a dependency, and instruct people who want to use it with Federalist to add fractal to their package.json. What do you think?

@hursey013 hursey013 force-pushed the bh-use-style-guide branch 2 times, most recently from 6345d62 to 39cb25d Compare August 2, 2017 21:45
@hursey013
Copy link
Member Author

@monfresh I modified the node package to only include the assets and not install any of the dependencies.

@monfresh
Copy link
Contributor

monfresh commented Aug 3, 2017

Awesome. Thanks!

@hursey013 hursey013 force-pushed the bh-use-style-guide branch from 39cb25d to 30999cd Compare August 3, 2017 14:51
@hursey013 hursey013 merged commit 574c5ac into master Aug 3, 2017
@hursey013 hursey013 deleted the bh-use-style-guide branch August 3, 2017 16:08
hursey013 added a commit that referenced this pull request Aug 4, 2017
This needs to be configured in Chef first.
hursey013 added a commit that referenced this pull request Aug 4, 2017
Revert "Merge pull request #1582 from 18F/bh-use-style-guide"
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