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] Add bulmaswatch w/ git auto-update via single package.json #11339

Merged
merged 1 commit into from
Jun 3, 2017
Merged

Conversation

jenil
Copy link
Contributor

@jenil jenil commented May 28, 2017

Pull request for issue: #10823
Related issue(s): #11322

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

  • Git repository (required): https://github.com/jenil/bulmaswatch
  • Official website (optional, not the repository):
  • NPM package url (optional):
  • License and its reference:
  • GitHub / Bitbucket popularity (required):
    • Count of watchers: 6
    • Count of stars: 107
    • Count of forks: 6
  • NPM download stats (optional):
    • Downloads in the last day:
    • Downloads in the last week:
    • Downloads in the last month:

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 😨 27969f9 CI test failed ❗

@jenil please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/8567 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.

@jenil congratulations! 6bddb78 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/8568, 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.

Nice work.
May I ask you how do you send this pull request? via GitHub website or use git command?
I think the commit should squash to one commit, if you use git command, we can help you how to do this.
But if you use GitHub website, it can't squash the commit.

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

BTY, Would you change the commit message to the followings?

Add bulmaswatch w/ git auto-update via single package.json

close #10823

This will helpful to us to check the git log, and when this PR close, it will close the related issue.

Thank you for your contribution.

"autoupdate": {
"source": "git",
"target": "git://github.com/jenil/bulmaswatch.git",
"fileMap": [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's let { move to next line.
Make same format as others.

"files": [
"bulmaswatch*.+(js|css|map)"
]
}]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's let ] move to next line.
Make same format as others.
the fileMap part as the followings:

    "fileMap": [
      {
        "basePath": "",
        "files": [
          "bulmaswatch*.+(js|css|map)"
        ]
      }
    ]

@jenil
Copy link
Contributor Author

jenil commented May 29, 2017

Hey @sufuf3!

I sent the PR using the Github website.
I tried using the git command but gave up since the repo was too huge. Even using the sparseChecout the repo is reasonably huge (2 gigs+), is there any other way to do this?

@sufuf3
Copy link
Contributor

sufuf3 commented May 30, 2017

No.
But we can help you to squash the commit after the package.json is fine. 😄

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 😨 3c868e5 CI test failed ❗

@jenil please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/8618 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 ☺️

@PeterDaveHello
Copy link
Contributor

@hare1039 help squash and correct commit message please.

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.

@jenil congratulations! f6ff703 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/8624, thank you 😀

"themes"
],
"autoupdate":
{
Copy link
Contributor

Choose a reason for hiding this comment

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

The position of this '{' is a little bit not in our format, but the review bot didn't catch it , so I think its fine.

{
"basePath": "",
"files": [
"bulmaswatch*.+(js|css|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 can't use this rule to fetch files from GitHub.
The file structure is [theme_name]/bulmaswatch.min.css and
[theme_name]/bulmaswatch.min.css.map

I'm not sure what rules should we apply here.
@pvnr0082t Please give some advices on this PR. Should we list all theme_names ?

Copy link
Contributor

@sufuf3 sufuf3 May 31, 2017

Choose a reason for hiding this comment

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

@hare1039 I forgot the basePath should be default
@jenil could you modified the auto-update part as the followings? I'm sorry that I not notice the basePath should be basePath to get the files.

  "autoupdate": {
    "source": "git",
    "target": "git://github.com/jenil/bulmaswatch.git",
    "fileMap": [
      {
        "basePath": "default",
        "files": [
          "bulmaswatch*.+(js|css|map)"
        ]
      }
    ]
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

@sufuf3 We only need default ?

Copy link
Contributor

@sufuf3 sufuf3 May 31, 2017

Choose a reason for hiding this comment

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

@hare1039 I think we can let author @jenil decide this.
But at first time, the author write the default/bulmaswatch.min.css as the main file(filename), so I think just need default as basePath.
ref: 6b13062

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sufuf3 @hare1039 I think we can't have default in the basePath since each theme has its own bulmaswatch.min.css. I had set the filename assuming that it is just a default file that I need to refer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jenil then let's use the auto-update as the followings, this will get all the theme's files:

  "autoupdate": {
    "source": "git",
    "target": "git://github.com/jenil/bulmaswatch.git",
    "fileMap": [
      {
        "basePath": "",
        "files": [
          "*/bulmaswatch*.+(js|css|map)"
        ]
      }
    ]
  }

And the "filename": "bulmaswatch.min.css", change to "filename": "default/bulmaswatch.min.css", let it as the main file.
Thank you.

Copy link
Contributor

@sufuf3 sufuf3 May 31, 2017

Choose a reason for hiding this comment

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

@jenil then let's use the auto-update as the followings, this will get all the theme's files:

  "autoupdate": {
    "source": "git",
    "target": "git://github.com/jenil/bulmaswatch.git",
    "fileMap": [
      {
        "basePath": "",
        "files": [
          "*/bulmaswatch*.+(js|css|map)"
        ]
      }
    ]
  }

And Let's change "filename": "bulmaswatch.min.css", to "filename": "default/bulmaswatch.min.css",

@sufuf3 sufuf3 changed the title Add bulmaswatch [new] [Author] Add bulmaswatch w/ git auto-update via single package.json May 31, 2017
@PeterDaveHello
Copy link
Contributor

@jenil any updates here or need help? Thanks.

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 😨 6a144e4 CI test failed ❗

@jenil please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/8703 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 ☺️

@PeterDaveHello
Copy link
Contributor

Oops, branch is too old, I'll fix it.

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.

@jenil congratulations! 086c605 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/8706, 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. 💯

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 2, 2017

@hare1039 @cdnjs/library-reviewer please help review this PR, thanks.

Copy link
Contributor

@hare1039 hare1039 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 d564b4f into cdnjs:master Jun 3, 2017
@PeterDaveHello
Copy link
Contributor

@jenil 👍

PeterDaveHello added a commit that referenced this pull request Jun 3, 2017
@jenil jenil deleted the bulmaswat branch June 5, 2017 09:38
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

6 participants