Skip to content
This repository has been archived by the owner on Jun 11, 2020. It is now read-only.

Updated vanilla to 1.5.0 from npm and rebuilt vanilla css files #942

Merged
merged 2 commits into from
Aug 31, 2017

Conversation

bartaz
Copy link
Contributor

@bartaz bartaz commented Aug 31, 2017

Done

Updates Vanilla to 1.5.0 from NPM.
Also updated CSS built from vanilla.

QA

  • Check out this feature branch
  • Run npm i (to update local npm modules)
  • vanilla-framework 1.5.0 should be installed
  • Run the site using the command npm start -- --env=environments/dev.env
  • View the site locally in your web browser at: http://0.0.0.0:8000/
  • Go to a page with breadcrumb (for example build details page)
  • Breadcrumb should have new vanilla styles (with blue links)

Issue / Card

Fixes #940

Screenshots

screen shot 2017-08-31 at 15 21 20

@bartaz bartaz requested a review from nottrobin August 31, 2017 13:22
@@ -1,3 +1,4 @@
@import 'vanilla-framework/scss/_utilities_clearfix.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.

Breadcrumb pattern was changed and now relies on %clearfix but doesn't import it on its own.

Copy link
Contributor

@nottrobin nottrobin left a comment

Choose a reason for hiding this comment

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

Works exactly as described, but I have one change request about version specificity in package.json

package.json Outdated
@@ -135,7 +135,7 @@
"style-loader": "^0.13.1",
"supertest": "^2.0.0",
"url-loader": "^0.5.7",
"vanilla-framework": "git+https://git@github.com/vanilla-framework/vanilla-framework.git#e3b7034a1caf39e4ed87b9481d5fe0e9913c2b0f",
"vanilla-framework": "^1.5.0",
Copy link
Contributor

@nottrobin nottrobin Aug 31, 2017

Choose a reason for hiding this comment

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

I'd only be comfortable with using this fuzzy versioning if we have a lockfile. I believe we're using NPM, rather than Yarn, for this project - is that right? In which case the lock-file to use would be package-lock.json. So could we either:

  • commit a package-lock.json file and make sure that the build job is using NPM>=5; or
  • lock all the dependencies in this file at specific versions

(Hopefully Yarn will one day come to its senses and start supporting package-lock.json)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait - it looks like we use npm-shrinkwrap to solve this problem. That's fine, but it means you need to be committing an update to the npm-shrinkwrap.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nottrobin I guess vanilla-framework didn't get into npm-shrinkwrap.json because it's a dev dependency (and dev dependencies are not locked in npm-shrinkwrap).

So if we want vanilla locked - and I agree it would be a good idea, we should move it to regular dependencies, so they will appear in npm-shrinkwrap.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bartaz yes I agree. Given that this site is never going to be an NPM package, the difference between dependencies and devdependencies is academic. So let's add it to regular deps.

@nottrobin
Copy link
Contributor

@bartaz with those changed it LGTM 👍

@bartaz bartaz merged commit 9b15a43 into master Aug 31, 2017
@nottrobin nottrobin deleted the vanilla-1.5.0 branch September 1, 2017 07:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants