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.

Sign up for GitHub

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

Jump to bottom

Issue #464: Replaces Grunt with NPM scripts #659

Merged
merged 10 commits into from
Oct 9, 2017
Merged

Issue #464: Replaces Grunt with NPM scripts #659

merged 10 commits into from
Oct 9, 2017

Conversation

jakeclements
Copy link
Contributor

@jakeclements jakeclements commented Sep 22, 2017 •
edited by matteofigus
Loading

Fixes #464

  • Replaces Grunt with NPM scripts
  • Removes text-limit on changelog messages
  • Updates contributing and readme docs with run-script commands

@matteofigus
Copy link
Member

matteofigus commented Sep 22, 2017 •
edited
Loading

wow @jakeclements this is an amazing work!
image

Give us some time to review but I just wanted to start saying thank you for this!

matteofigus
matteofigus approved these changes Sep 26, 2017
View reviewed changes
Copy link
Member

@matteofigus matteofigus left a comment

Choose a reason for hiding this comment

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

This LGTM. We are in the process of merging a couple of urgent PRs, but let's merge this as soon as possible after those.

@jakeclements
Copy link
Contributor Author

Thanks for the review @matteofigus, say the word once your PRs are in and I'll merge this.

@nickbalestra
Copy link
Contributor

Wohoo, super excited for this.
Thanks 💯 @jakeclements for the 💅 😍 , awesome stuff!

nickbalestra
nickbalestra approved these changes Sep 27, 2017
View reviewed changes
Copy link
Contributor

@nickbalestra nickbalestra left a comment

Choose a reason for hiding this comment

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

👍

matteofigus
matteofigus requested changes Oct 5, 2017
View reviewed changes
Copy link
Member

@matteofigus matteofigus left a comment

Choose a reason for hiding this comment

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

@jakeclements I did a final review for merging but found just 2 little things. Can you have a look, if you can work on those it would be nice, if not I can still merge it and we can make the changes in subsequent PRs. Thanks and sorry for taking long for reviewing 😢

CONTRIBUTING.md Outdated
@@ -24,11 +24,11 @@ You need to be enabled for doing this.
* `master` should be all green. If not, make it green first.
* git checkout master
* git pull --tags
* Run `grunt version:<versionType>` for new version.
* Run `npm run version -- --type="<versionType>"` for new version.
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that for the sake of simplicity, we could simplify this by having npm run patch, npm run minor and npm run major? What do you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, the current script is unnecessarily verbose.
I'm happy to update this so we can keep it within a single PR.

package-lock.json Outdated
@@ -0,0 +1,11339 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We gitignored the package-lock in master, can you sync with that so we can remove this?

@jakeclements
Copy link
Contributor Author

@matteofigus thanks for the review, no problem about the timeframe.
I've updated based on the requested changes, take a look when you get a chance and let me know if you would like me to merge.

matteofigus
matteofigus approved these changes Oct 9, 2017
View reviewed changes
Copy link
Member

@matteofigus matteofigus left a comment

Choose a reason for hiding this comment

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

💣 💥 Thanks!!!

@matteofigus matteofigus merged commit 70b04ac into opencomponents:master Oct 9, 2017
@matteofigus
Copy link
Member

Welcome to the contributors! 👍

@matteofigus matteofigus mentioned this pull request Oct 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matteofigus matteofigus matteofigus approved these changes

@nickbalestra nickbalestra nickbalestra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakeclements @matteofigus @nickbalestra