Skip to content

Update change log#925

Merged
msecret merged 7 commits into
v0.9.xfrom
update-changelog
Dec 21, 2015
Merged

Update change log#925
msecret merged 7 commits into
v0.9.xfrom
update-changelog

Conversation

@maya

@maya maya commented Dec 11, 2015

Copy link
Copy Markdown
Contributor

This updates the change log to include changes for the next release v0.9.0.

TODO:

  • add all changes made before release

@maya maya changed the title Update change log [WIP] Update change log Dec 21, 2015
msecret pushed a commit that referenced this pull request Dec 21, 2015
@msecret msecret merged commit 7d27c40 into v0.9.x Dec 21, 2015
Comment thread CHANGELOG.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jeremiak did we completely figure out npm support? Like could I currently do npm install and @import 'wds' and it'll work?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, we're using it on Federalist but we run the build task on our side and then we move that CSS around. See scripts here

I'm curious, what would you expect if you were to import 'wds'?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yea I guess its two different use cases.

import "wds"; would import the scss file which you then compile. There's also the use case of like require('wds') using webpack, which could be the CSS. We'd potentially have a different npm module for the Sass files, I've seen this done before.

For when we're publishing this to npm, I don't think you should have to build it to get the CSS, which is what federalist is doing now. You should npm install it, and the CSS should be compiled somehwere, and then I think there's even something like a "main-css" directive you can put in package.json to point to it.

Also, what is the plan to publish this on npm?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the approach that Bootstrap uses:

  • There will be a css and a scss key in the package.json that each point to a file. The main key in the package.json will point to the main javascript for the library. This means that if you require('uswds') you will get the main JavaScript file, not the CSS.
  • To get to the css and scss keys programatically one could require('uswds/package.json').css or require('uswds/package.json').scss

I don't think we should commit compiled code to Github, instead using npm prepublish scripts and maybe even having Travis build and publish when appropriate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jeremiak: @alexose mentioned liking having pre-built files in the repo so people can clone and get started right away. He also mentioned D3 as a good example that does this. cc @msecret

@maya maya deleted the update-changelog branch December 22, 2015 21:04
@mbland

mbland commented Jan 11, 2016

Copy link
Copy Markdown
Contributor

Since I've scrubbed in to try to help @maya with this, I've run into some questions for @jeremiak and @msecret.

So when we talk about "npm support", we're talking about being able to write browser-only JavaScript that can require('uswds'), ostensibly after browserifying it? We're not talking about being able to require('uswds') from an arbitrary Node program or the Node REPL and having it work without errors? For example:

// npm.js
// Compiled using: browserify npm.js -o index.js
'use strict';
require('uswds');

And then in the HTML file:

<script src="./assets/js/vendor/jquery-1.11.3.min.js"></script>
<script src="./index.js"></script>

This seems to behave in my browser; whereas require('uswds') in the Node REPL dies horribly.

And these programmatic require('uswds/package.json').css or require('uswds/package.json').scss directives aren't actually standardized yet, but proposed standards, correct? I saw that bootstrap uses the less and style tags, but gives no guidance on how they're used. Then I found that the parcelify npm supposedly uses style, but again, that requires the user knowing to use it. Also:

Point being, if we add these keys, we'll have to document how to use them. So basically, the package.json needs updated to something like this, plus some documentation, and we're good to go:

  "main": "assets/js/components.js",
  "scripts": {
    "build-sass": "node-sass assets/_scss/all.scss > assets/css/uswds.css",
    "prepublish": "npm run build-sass build"
  },
  "assets": "assets/",
  "css": "assets/css/uswds.css",
  "scss": "assets/_scss/all.scss",

mbland pushed a commit that referenced this pull request Jan 11, 2016
@jeremiak

Copy link
Copy Markdown
Contributor

Cool, I'll take a look at the PR @mbland but also wanted to answer your question. In my opinion the benefit of having the library on NPM is that it can be a declared dependency to ease installation and deployment. If you are var wds = require('uswds')-ing then I would assume that wds is an object from the index.js file in that repo and be the JavaScript components of the library.

As far as those other keys, they're definitely not yet standards with regards to their semantic meaning or place in package.json. But 👍 for shipping it with documentation, as it enables flexibility for any arbitrary build tool chain.

As an example, on Federalist we use the build-sass script to compile the CSS.

@mbland

mbland commented Jan 12, 2016

Copy link
Copy Markdown
Contributor

No, I get the benefit of npm packaging. I was checking whether my understanding of how it would be used is (now) correct, that it would be required by browser scripts only. This after pulling a fair amount of hair out trying to get it to load without throwing a stack trace in the REPL.

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.

4 participants