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

add dom-to-image w/ npm auto-update #8701

Merged
merged 2 commits into from Nov 7, 2016

Conversation

kennynaoh
Copy link
Contributor

@kennynaoh kennynaoh commented Aug 8, 2016

PR for #8589
@x09326 please help me review this pull request, thank you.
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/tsayen/dom-to-image
  • Official website (optional, not the repository):
  • NPM package url (optional): https://www.npmjs.com/package/dom-to-image
  • GitHub / Bitbucket popularity (required):
    • Count of stars: 182
    • Count of watchers: 9
    • Count of forks: 46
  • NPM download stats (optional):
    • Downloads in the last day: 11
    • Downloads in the last week: 184
    • Downloads in the last month: 770

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.

@kennynaoh kennynaoh changed the title Dom to image add dom-to-image@2.5.1 w/ npm auto-update Aug 9, 2016
@pvnr0082t
Copy link
Contributor

@PeterDaveHello I think this PR is ok. Please review it again, thank you.

@PeterDaveHello
Copy link
Contributor

Some versions have different naming for the src and dist file, would you please make them the same? Maybe could ask the author for suggestions, thanks.

@kennynaoh kennynaoh changed the title add dom-to-image@2.5.1 w/ npm auto-update add dom-to-image w/ npm auto-update Nov 2, 2016
@kennynaoh
Copy link
Contributor Author

The author said domvas is a separate project, this lib is originally it's fork, so I think we don't need to add this file under src/ in some of old versions.
@pvnr0082t , @x09326
Please help me review it again.
Thanks!

Copy link
Contributor

@pvnr0082t pvnr0082t left a comment

Choose a reason for hiding this comment

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

GitHub show that this branch is out-of-date with the base branch. Please rebased on the latest master branch, thanks. @kennynaoh

@pvnr0082t pvnr0082t removed their assignment Nov 2, 2016
@pvnr0082t
Copy link
Contributor

LGTM

@x09326
Copy link
Contributor

x09326 commented Nov 7, 2016

@kennynaoh
Please rebase with the latest master branch, thanks.

Manually add the other versions from git repo because npm auto-update
can't get some of the old versions, cc cdnjs#8589
@kennynaoh
Copy link
Contributor Author

@pvnr0082t , @x09326
I have rebased to the latest master br.
Please help me review it again.
Thanks!

@pvnr0082t
Copy link
Contributor

LGTM

@PeterDaveHello PeterDaveHello merged commit b23d0d9 into cdnjs:master Nov 7, 2016
@kennynaoh kennynaoh deleted the dom-to-image branch December 7, 2016 06:31
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

4 participants