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 [email protected] w/ git auto-update #12158

Merged
merged 2 commits into from
May 11, 2018

Conversation

sashberd
Copy link
Contributor

close #8979, cc @wikimedia

Pull request for issue: #8979
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.

@ghost ghost assigned sashberd Nov 12, 2017
@ghost ghost added the in progress label Nov 12, 2017
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.

@sashberd congratulations! 2c75cc9 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/14988, thank you 😀

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.

Other than the following request, I think you should open a issue to check that is there stable version(s) they can release. The current latest one 1.0.0+20131229 seem not stable. The commit number of that one is 806 (See https://github.com/wikimedia/jquery.ime/tree/1.0.0+20131229). The commit number of master branch is 1065. There might be some versions they didn't tag/release yet.

{
"basePath": "",
"files": [
"!(README)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this rule. It fetches so many files CDN doesn't need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my mistake with pattern the !(README) should be included inside rules see here wikimedia/jquery.ime#453

"basePath": "",
"files": [
"!(README)",
"rules/**/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need

  1. css/jquery.ime.css. Fetch it using the rule css/jquery.ime* (you can know this through .html file under example)
  2. Because (1) is needed, all files under images should also be added too, use the rule images/*

@sashberd
Copy link
Contributor Author

sashberd commented Nov 21, 2017

@santhoshtr @amire80 Following @extend1994 question about stable version release... Could you explain and help with this?
Thanks

@sashberd
Copy link
Contributor Author

@extend1994 The PR was updated. FYR

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.

@sashberd congratulations! 0fffefd 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/15311, 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.

@sashberd Thanks for your contribution.

  1. Would you discuss the git tag issue with the author?
    https://github.com/wikimedia/jquery.ime/releases
    The version name 1.0.0+20131229 is very weird.
    Also, after this PR be merged, bot still add 1.0.0+20131229 version.
    I think we have to exclude this 1.0.0+20131229 version via gitignore, but first of all, we have to get the good version name.

  2. Should we need images/ime-inactive.svg & images/ime-inactive.png?

  3. images/ime-active.svg & images/tick.svg is different from upstream (use md5sum chceck)

Many thanks.

@@ -0,0 +1,217 @@
.imeselector {
Copy link
Contributor

Choose a reason for hiding this comment

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

The file structure should be

├── css
│   └── jquery.ime.css

Many thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I opened an issue. lets wait for the author answer
  2. We need this files.. See the demo pages. It is used for show right bottom corner querty image
  3. The files were updated from the stream with wget

@sashberd
Copy link
Contributor Author

@sufuf3 The PR was updated. See comments above. FYR

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.

@sashberd congratulations! 587a528 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/16257, thank you 😀

@sashberd sashberd changed the title Add [email protected] w/ git auto-update Add [email protected] w/ git auto-update Feb 9, 2018
@sashberd
Copy link
Contributor Author

sashberd commented Feb 9, 2018

@sufuf3 The authors added new normal version 0.2.0 see [here] (wikimedia/jquery.ime#505) The PR was updated. FYR

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

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

@sashberd congratulations! 18cab92 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/17867, thank you 😀

@sufuf3
Copy link
Contributor

sufuf3 commented Feb 12, 2018

@sashberd As wikimedia/jquery.ime#505 (comment) comment, would you please update .gitignore file?
Many 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.

@sashberd congratulations! 6dd67f8 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/17956, thank you 😀

@sufuf3
Copy link
Contributor

sufuf3 commented Feb 14, 2018

In the second commit message title Update .gitignore file, exclude v1.0.0+20131229, I don't know which lib need to be excluded v1.0.0+20131229.
Would you please mention the library name?
Many thanks.

Ref: https://github.com/cdnjs/cdnjs/search?q=gitignore&type=Commits&utf8=%E2%9C%93

@sashberd
Copy link
Contributor Author

@sufuf The PR was updated. FYR

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.

@sashberd congratulations! 8094d82 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/18027, thank you 😀

@PeterBot
Copy link
Contributor

Any updates here? Please let me know if you need any help. Thanks!

(This is an automatic ping message, sorry for disturbing, we will get back here ASAP.)

cc @sashberd

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.

  • The first commit message
    Would you please fix the typo minifing?
  • The second commit message
    Let's revise from
    Update .gitignore file, exclude v1.0.0+20131229 of juery.ime lib to Update .gitignore to exclude [email protected]+20131229 to follow 50/72 rule closer.

Many thanks.

@sufuf3 sufuf3 changed the title Add [email protected] w/ git auto-update [New] Add [email protected] w/ git auto-update Apr 25, 2018
@sashberd sashberd requested a review from sufuf3 April 26, 2018 11:46
@sashberd
Copy link
Contributor Author

@sufuf3 The commit messages were revised. FYR

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 😨 91f22fb CI test failed ❗

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

@sashberd congratulations! 34ff55b 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/20045, 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.

@sashberd Thanks for the PR.
Some files are different from upstream.

  • In this PR
7ab007ba9302957722e03444d8063da2  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-todo.js
a939bc31d22b5c33aee6b8d62df982df  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-tradali.js
fac8a594afbfa52913031c77bc324985  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-trad.js
a939bc31d22b5c33aee6b8d62df982df  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-tradali.js
e9df25e99535c8d8cb33bcbb9fe812f0  ajax/libs/jquery.ime/0.2.0/rules/mn/phagspa.js
6f0946d2ca6d9e188f352796a2fdd39d  ajax/libs/jquery.ime/0.2.0/rules/mnc/mnc.js
b952fada91541cb6b8f7248e2d2e12f5  ajax/libs/jquery.ime/0.2.0/rules/mnc/mnc-ali.js
a82f0bafd7f3bdfe4ccabc6c55d326b8  ajax/libs/jquery.ime/0.2.0/rules/sjo/sjo.js
  • Upstream
72fb8d9b79b76fb007d3e8aebc392e77  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-todo.js
8c675e67d2e5b410048d9c26c0a65dcc  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-todoali.js
671cabda7f5f87b20ceff18007ceaf1b  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-trad.js
a6c3c3e6619f6c5a22871085de5e6127  ajax/libs/jquery.ime/0.2.0/rules/mn/mn-tradali.js
bbcc01e272af6fb9291b1de031ac84a3  ajax/libs/jquery.ime/0.2.0/rules/mn/phagspa.js
d384b6758040a37016a903f1bd5b1684  ajax/libs/jquery.ime/0.2.0/rules/mnc/mnc-ali.js
27d099da007701b945d9c16c7ec919a8  ajax/libs/jquery.ime/0.2.0/rules/mnc/mnc.js
cb20b74e57c93a0104f3d6d44d2e1b26  ajax/libs/jquery.ime/0.2.0/rules/sjo/sjo.js

Would you please re-add them and re-generate the minified files.
Thank you.

"localization",
"ime",
"jquery",
"l10n"
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 add more keywords(like intl) to help people to search this lib.
screen shot 2018-04-27 at 11 02 06 am
Thanks

"type": "git",
"url": "git://github.com/wikimedia/jquery.ime.git"
},
"filename": "jquery.ime.min.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 move this line in second order.
To help people see it at the first glance.
Thank you.

@sashberd
Copy link
Contributor Author

@sufuf3 The PR was updated. FYR

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.

@sashberd congratulations! 9878e18 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/20127, 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
Thanks for the contribution.

PS.
I got the files using the auto-updater and generated the minified files. However, I found some files are different from my result. These files are as the following:

        modified:   ajax/libs/jquery.ime/0.2.0/jquery.ime.min.js
        modified:   ajax/libs/jquery.ime/0.2.0/jquery.ime.min.js.map
        modified:   ajax/libs/jquery.ime/0.2.0/jquery.ime.selector.min.js
        modified:   ajax/libs/jquery.ime/0.2.0/jquery.ime.selector.min.js.map
        modified:   ajax/libs/jquery.ime/0.2.0/rules/bo/bo-sambhota.min.js

Not sure the cause is from the version of minified tool.

I have forgotten to reminder cc #8979 in the second commit, but I think maybe it fine here.

@sufuf3 sufuf3 requested a review from PeterDaveHello May 3, 2018 03:19
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.

@sashberd congratulations! e452665 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/20417, thank you 😀

@PeterDaveHello PeterDaveHello merged commit 2d8f800 into cdnjs:master May 11, 2018
@ghost ghost removed the in progress label May 11, 2018
@sashberd sashberd deleted the jquery.ime branch May 13, 2018 08:59
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.

[Request] Add jquery.ime
5 participants