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

[author][new] Add billboard.js w/ npm auto-update via single package.json #11413

Merged
merged 1 commit into from
Jun 22, 2017
Merged

Conversation

netil
Copy link
Contributor

@netil netil commented Jun 16, 2017

Pull request for issue: #11409
Related issue(s):

Checklist for Pull request or lib adding request issue follows the conventions.

Note that if you are using a distribution purpose repository/package, please also provide the url and other related info like popularity of the source code repo/package.

Profile of the lib

Essential checklist

  • I'm the author of this library
    • I would like to add link to the page of this library on CDNJS on website / readme
  • This lib was not found on cdnjs repo
  • No already exist / duplicated issue and PR
  • The lib has notable popularity
    • More than 100 [Stars / Watchers / Forks] on [GitHub / Bitbucket]
    • More than 500 downloads stats per month on npm registry
  • Project has public repository on famous online hosting platform (or been hosted on npm)

Auto-update checklist

  • Has valid tags for each versions (for git auto-update)
  • Auto-update setup
  • Auto-update target/source is valid.
  • Auto-update filemap is correct.

Git commit checklist

  • The first line of commit message is less then 50 chars, be clean and clear, easy to understand.
  • The parent of the commit(s) in the PR is not old than 3 days.
  • Pull request is sending from a non-master branch with meaningful name.
  • Separate unrelated changes into different commits.
  • Use rebase to squash/fixup dummy/unnecessary commits into only one commit.
  • Close corresponding issue in commit message
  • Mention related issue(s), people in commit message, comment.

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 8794481 CI test failed ❗

@netil please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/9197 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@netil congratulations! 90091b7 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9198, thank you 😀

Copy link
Contributor

@dakshshah96 dakshshah96 left a comment

Choose a reason for hiding this comment

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

Please make the requested changes.

Also, if you're working locally and not directly from the GitHub interface, make the following changes:

  • Squash your commits into a single commit
  • Use the following commit message:
Add billboard.js w/ npm auto-update via single package.json

close #11049

If you're not working locally, don't worry about these. Our maintainers will help you with it.

"description": "Re-usable easy interface JavaScript chart library, based on D3 v4+",
"author": "NAVER Corp.",
"filename": "billboard.min.js",
"homepage": "http://naver.github.io/billboard.js/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use https here.

{
"basePath": "dist",
"files": [
"**/!(*.+(map))"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to exclude the map files. Seems fine to use **/* inside dist.

@dakshshah96 dakshshah96 changed the title [author][new] Add billboard.js@1.0.0 w/ npm auto-update [author][new] Add billboard.js w/ npm auto-update via single package.json Jun 16, 2017
@netil
Copy link
Contributor Author

netil commented Jun 16, 2017

@dakshshah96 thanks. I did requested changes (sorry not squashing, I did changes directly from web)

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@netil congratulations! 3846562 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9203, thank you 😀

Copy link
Contributor

@sufuf3 sufuf3 left a comment

Choose a reason for hiding this comment

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

LGTM
Just need to squash to one commit.
Would you want to try squash to one commit by yourself?

If you want to try to use git command but worried cdnjs's repo is too huge. You could use sparseCheckout, it lets you check out only the files you want. Here is the tutorial: https://github.com/cdnjs/cdnjs/blob/master/documents/sparseCheckout.md

OR, I can help you squash to one commit.

Thank you for your contribution.

@netil
Copy link
Contributor Author

netil commented Jun 19, 2017

@sufuf3, would you mind to squash?

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 49d78ba CI test failed ❗

@netil please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/9277 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 a0cac9c CI test failed ❗

@netil please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/9279 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@netil congratulations! 8f64e56 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9288, thank you 😀

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@netil congratulations! 351e452 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/9289, thank you 😀

@sufuf3
Copy link
Contributor

sufuf3 commented Jun 19, 2017

@netil Done 😄
Help squash, rebase to master, modified line 26 from ] to ] (delete two space which in the end of this line).

Copy link
Contributor

@sufuf3 sufuf3 left a comment

Choose a reason for hiding this comment

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

LGTM

@sufuf3
Copy link
Contributor

sufuf3 commented Jun 20, 2017

@cdnjs/intern3 @dakshshah96
Please help review this PR, Thank you.

Copy link
Contributor

@yahsieh yahsieh left a comment

Choose a reason for hiding this comment

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

LGTM

@sufuf3
Copy link
Contributor

sufuf3 commented Jun 21, 2017

@cdnjs/library-reviewer
Please help review this PR, Thank you.

Copy link
Contributor

@dakshshah96 dakshshah96 left a comment

Choose a reason for hiding this comment

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

LGTM

@PeterDaveHello PeterDaveHello merged commit b6a5d04 into cdnjs:master Jun 22, 2017
PeterDaveHello added a commit that referenced this pull request Jun 22, 2017
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.

None yet

7 participants