Update CONTRIBUTING with copy from release notes#1038
Conversation
|
@kategarklavs I only added to what was already here in the contributing docs, but if you could take a look at this that'd be |
|
@rogeruiz Taking a look now! |
| @@ -46,50 +46,60 @@ Here are a few guidelines to follow when submitting a pull request: | |||
|
|
|||
| Questions or need help with setup? Feel free to open an issue here [https://github.com/18F/web-design-standards/issues](https://github.com/18F/web-design-standards/issues). | |||
There was a problem hiding this comment.
I'd replace "Questions" with "Have questions or ..."
Also, to promote concision, I'd remove the "Feel free to" and start the second sentence with "Open an issue here:"
|
@rogeruiz Hey! Just took a look and made a few comments/recommendations. If you have any questions, please send them my way! |
| supported via `npm`. This package contains no documentation and the compiled | ||
| and bundled CSS, JavaScript, fonts, and images. To build the package locally, | ||
| run the following command: | ||
| Here are a few of the common tasks: |
There was a problem hiding this comment.
Ah, OK: So, as I was reading this, I could see how a user might get confused by the statement "Here are a few of the common tasks," which is followed by one example (gulp build).
What if we were to reword this to "The following examples cover a few common tasks" -OR- "The following examples detail a few tasks you'll encounter as you use gulp" — something along those lines?
d3d7505 to
a0b2882
Compare
This patch addresses the comments here: #1038 (comment)
This patch addresses the comment @msecret brought up here: #1038 (comment)
|
@kategarklavs and @msecret I've updated the branch and addressed your comments 🎉 let me know if there is anything else this needs before we can merge it in. Thanks! 🚀 |
|
@rogeruiz Checking now! Thanks for the heads up. |
| supported via `npm`. This package contains no documentation and the compiled | ||
| and bundled CSS, JavaScript, fonts, and images. To build the package locally, | ||
| run the following command: | ||
| The following examples detail a few tasks you'll encounter as you use gulp |
There was a problem hiding this comment.
Please add a colon after "as you use gulp," so the sentence will read as follows:
The following examples detail a few tasks you'll encounter as you use gulp:
|
@rogeruiz OK, just took a look and made one comment about adding a colon — aside from that, this looks good to go 👍 |
This patch addresses the comments here: #1038 (comment)
This patch updates the `bundle` command to be an install like the comments say rather than an update.
This patch addresses the comment @msecret brought up here: #1038 (comment)
b26f535 to
23736b2
Compare
| "main": "src/js/start.js", | ||
| "scripts": { | ||
| "prepublish": "gulp build", | ||
| "prepublish": "bundle && gulp build", |
There was a problem hiding this comment.
I personally think that we shouldn't require ruby as a dependency to work on the project and we should just put gulp no-test build here, but am fine with this for now.
There was a problem hiding this comment.
yeah I agree with this, but the /docs directory is still a Ruby project so the Gemfile being there is what makes it a ruby project. 0.2% is still something I guess.
|
LGTM |
Update CONTRIBUTING with copy from release notes
I ported over the
gulp workflowsection from the release notes into what was already in CONTRIBUTING. I updated the release notes here with the a reference to this update.