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

[New] Add vue-instant@1.0.2 w/ npm auto-update #12027

Merged
merged 1 commit into from Feb 9, 2018

Conversation

venkatramanareddy
Copy link
Contributor

@venkatramanareddy venkatramanareddy commented Oct 21, 2017

Pull request for issue: #11975
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 200 [Stars / Watchers / Forks] on [GitHub / Bitbucket]
    • More than 800 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.

@venkatramanareddy congratulations! 0a2f9bb 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/14112, 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.

@venkatramanareddy congratulations! ba47539 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/14501, 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
@venkatramanareddy Nice work.
Helped update commit message because the author is santiblanko not verglor and minify the .css file.

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

@sufuf3 sufuf3 changed the title Add vue-instant@1.0.2 w/ npm auto-update [New] Add vue-instant@1.0.2 w/ npm auto-update Oct 31, 2017
Copy link
Contributor

@extend1994 extend1994 left a comment

Choose a reason for hiding this comment

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

Wondering vue-instant.common.js is for a browser? It seems for commonjs.
@venkatramanareddy @sufuf3 Do you know about this?

@sufuf3
Copy link
Contributor

sufuf3 commented Nov 5, 2017

@extend1994 I think we can discuss this.
I know that vue-instant.common.js should be used in CommonJS environment not be used in browser. (https://github.com/santiblanko/vue-instant/blob/master/config/webpack.config.common.js#L15)

But as I know there are another ways can let CommonJS/Node.js require module loader for the browser, such as https://github.com/Stuk/require1k or https://github.com/ruanyf/tiny-browser-require.

Then, we still can use it in front-end (https://webpack.github.io/docs/usage.html).

But I am still a new hand to learning Javascript bundler.

@PeterDaveHello Could you give us some suggestion in this issue?
Many Thanks.

@PeterDaveHello
Copy link
Contributor

Ask its author or find the answer from the doc please, I can't know every case.

Copy link
Contributor

@maruilian11 maruilian11 left a comment

Choose a reason for hiding this comment

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

And please remove 1.0.2/vue-instant.common.js. Thanks!

{
"basePath": "dist",
"files": [
"**/*"
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 "!(vue-instant.common.js)" rather than "**/*" in this case because vue-instant.common.js can't be used in browser.

@maruilian11
Copy link
Contributor

And please modify your commit msg to:

Add vue-instant@1.0.2 w/ npm auto-update

And minify vue-instant.css with web-minify-helper
close #11975, cc @santiblanko

The msg body should be wrapped in 72 characters.

@extend1994
Copy link
Contributor

@venkatramanareddy Would you like to continue this PR?

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.

@venkatramanareddy congratulations! 9942d57 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/17437, thank you 😀

@venkatramanareddy
Copy link
Contributor Author

@extend1994 I've done the necessary changes. Apologies for the delay

Copy link
Contributor

@extend1994 extend1994 left a comment

Choose a reason for hiding this comment

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

Sorry for late review!

{
"basePath": "dist",
"files": [
"!(vue-instant.common.js)"
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 use **/!(vue-instant.common.js) instead, it can fetches files under sub-directories compared to the original one.

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.

@venkatramanareddy congratulations! 4e09d97 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/17658, thank you 😀

extend1994
extend1994 previously approved these changes Feb 9, 2018
@extend1994 extend1994 dismissed their stale review February 9, 2018 13:45

Can't use "Rebase and merge". Going to rebase from my side to approve and merge this PR

And minify vue-instant.css with web-minify-helper
close cdnjs#11975, cc @santiblanko
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.

@venkatramanareddy congratulations! 347351e 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/17840, thank you 😀

@extend1994 extend1994 merged commit fa5e317 into cdnjs:master Feb 9, 2018
@ghost ghost removed the in progress label Feb 9, 2018
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