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 Infusion@2.0.0 w/ npm auto-update #10309

Merged
merged 2 commits into from Jan 31, 2017

Conversation

waharnum
Copy link
Contributor

@waharnum waharnum commented Jan 16, 2017

This PR adds the latest version of Infusion, a Javascript framework used by a number of international research teams working on accessible software projects and in a number of other educational contexts.

I'm one of the contributors and suggested adding it to cdnjs in this thread on our work mailing list: http://lists.idrc.ocad.ca/pipermail/fluid-work/2017-January/010222.html


Profile of the lib

Essential checklist

  • I'm the author of this library (contributor)
    • 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)

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

@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.

Could you please apply auto-update config in package.json? You can take a look at auto-update.md.

"dependencies": {
"fluid-resolve": "1.2.0"
},
"license": "(BSD-3-Clause OR ECL-2.0)",
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 licenses array as following:

  "licenses": [
    "BSD-3-Clause",
    "ECL-2.0"
  ],

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.

Could you please also commit source files? Thanks.

@waharnum
Copy link
Contributor Author

waharnum commented Jan 17, 2017

@pvnr0082t - by source files, do you mean the unminified consolidated files, or the individual source files for different parts of the framework that are available at https://github.com/fluid-project/infusion/tree/master/src?

@pvnr0082t
Copy link
Contributor

@waharnum I meant the former. Sorry for confusing you.

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.

@waharnum Could you please help minify the css files under assets/src/framwork/preferences/css? Thanks

@waharnum
Copy link
Contributor Author

@pvnr0082t minified CSS added - we don't currently minify JS/CSS for our /dist directory in NPM, but I will work on making that a part of our publish process to NPM for the next version.

@pvnr0082t
Copy link
Contributor

@waharnum Okay. Nice work!

@pvnr0082t pvnr0082t changed the title [new] Add Infusion 2.0.0. [new] Add Infusion@2.0.0 w/ npm auto-update Jan 17, 2017
@waharnum
Copy link
Contributor Author

@pvnr0082t thank you for speedy review - I'm excited to be able to put Infusion on cdnjs!

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.

LGTM

@waharnum
Copy link
Contributor Author

waharnum commented Jan 27, 2017

Hello - checking in whether there is further work needed from our end to move this forward? I believe I've addressed all @pvnr0082t's comments. It would be great to have 2.0.0 release available on the CDN.

@pvnr0082t
Copy link
Contributor

ping @kennynaoh

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.

Hi @waharnum, could you please also remove dependencies since we don't need it.
And please modify all files permission to -rw-r--r--.
Thanks!

@waharnum
Copy link
Contributor Author

Hi @maruilian11 - I've removed the dependencies block in package.json.

Regarding file permissions, I don't see any files that have permissions other than -rw-r--r-- (at least on my local checkout) except for the directories, which have the standard drwxr-xr-x permission structure. Is there a specific file or files that are an issue? Sorry if I'm missing something obvious!

@maruilian11
Copy link
Contributor

maruilian11 commented Jan 31, 2017

@waharnum sorry, I think the permission is ok now. could you rebase this PR with our latest master branch by using git rebase master since the previous commit is too old. Thanks!

@PeterDaveHello
Copy link
Contributor

@maruilian11 You should confirm that the contributor is using a GitHub gui or a local command line first before asking rebase ... btw, for non-active users without willing to clone the repo for just rebase, I think you should try to help rebase (rebase by you) even with a squash/fixup process.

@maruilian11
Copy link
Contributor

maruilian11 commented Jan 31, 2017

@PeterDaveHello Got it.
@waharnum I rebase this branch with the latest master branch and squash the commits, I hope you won't mind I made this change.

@PeterDaveHello
Copy link
Contributor

@maruilian11 your sentence looks strange ...

@maruilian11
Copy link
Contributor

LGTM

@maruilian11
Copy link
Contributor

@PeterDaveHello hmm... you mean the sentence I said to waharnum?

@PeterDaveHello
Copy link
Contributor

@maruilian11 check it yourself ...

@maruilian11
Copy link
Contributor

@PeterDaveHello Just modify it. Could you help me check this PR?

@PeterDaveHello
Copy link
Contributor

@maruilian11 I just don't get it, why don't you ask the contributor before the rebasing? I don't believe that is how the open source community works.

@maruilian11
Copy link
Contributor

maruilian11 commented Jan 31, 2017

@PeterDaveHello hmm... Because I think the PR only need to be rebased, I thought it would be helpful....
I will ask the contributor first next time... sorry...

@waharnum
Copy link
Contributor Author

No problem on the rebase, thank you @maruilian11. Anything further needed on this one to make the PR acceptable? Thanks again!

@PeterDaveHello
Copy link
Contributor

PeterDaveHello commented Jan 31, 2017

Looks like the most versions on npm package are not for production but development builds ... so should update .gitignore to ignore them.

  • 2.0.0-dev.20151007T134312Z.8c33cd4
  • 2.0.0-dev.20151024T031903Z.3da18aa
  • 2.0.0-dev.20151027T140548Z.c9d974c
  • 2.0.0-dev.20151103T162114Z.69aa6c8
  • 2.0.0-dev.20151103T223642Z.130b29c
  • 2.0.0-dev.20151113T224751Z.3ba00f2
  • 2.0.0-dev.20151123T190910Z.35b21fd
  • 2.0.0-dev.20151130T180545Z.18ee3f8
  • 2.0.0-dev.20151211T025229Z.eabb4fc
  • 2.0.0-dev.20160115T165518Z.2f38c09
  • 2.0.0-dev.20160309T145652Z.9f515ec
  • 2.0.0-dev.20160505T142427Z.63b8a53
  • 2.0.0-dev.20160513T002747Z.477b81a
  • 2.0.0-dev.20160519T222603Z.754d2c6
  • 2.0.0-dev.20160902T155354Z.b963420
  • 2.0.0-dev.20160911T222604Z.b86ecbb
  • 2.0.0-dev.20161002T002156Z.a2802df
  • 2.0.0-dev.20161027T155641Z.0bcd507
  • 2.0.0-dev.20161124T191742Z.17a345b
  • 2.0.0-dev.20161219T154515Z.0f2ccc8
  • 2.0.0-dev.20161219T170555Z.5778f7e
  • 3.0.0-dev.20170118T192406Z.368093e
  • 3.0.0-dev.20170127T130413Z.103de6e
  • 3.0.0-dev.20170128T013435Z.82e259f
  • 3.0.0-dev.20170131T153243Z.6aab53a

@PeterDaveHello PeterDaveHello merged commit 901c505 into cdnjs:master Jan 31, 2017
@waharnum
Copy link
Contributor Author

@PeterDaveHello - yes, the nature of our work means dev packages are published to NPM so that they can be sourced from other work. I was going to add a blanket *-dev.* ignore but see you have already done so. Thanks!

I have an outstanding PR to generate minified /dist files automatically for our NPM publish process that will be in before our next major release.

Thank you for the merge!

@PeterDaveHello
Copy link
Contributor

@waharnum no problem, thanks for your contribution.

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