longbill / jquery-date-range-picker

A jQuery plugin that allows user to select a date range
MIT License
1.12k stars 579 forks source link

Greek language import #514

Closed ilaydaozdemir99 closed 2 years ago

ilaydaozdemir99 commented 2 years ago

511 Greek language has been added.

holtkamp commented 2 years ago

Thanks @ilaydaozdemir99, merged it and tried to build the project, however it seems I am running into some problems due to outdated packages, such as https://stackoverflow.com/questions/55921442/how-to-fix-referenceerror-primordials-is-not-defined-in-node-js

@jcubic do you have any up-to-date knowledge on how to get the project "building" again with modern versions of NPM and Gulp?

monovertex commented 2 years ago

The dependencies most likely need an older Node version. Updating the library to modern versions is a bit more involved and bigger of an effort.

The last time I built the library I used an older version of Node.

I’ll build the library tomorrow morning, as I am away from my laptop right now.

On Sun, 12 Dec 2021 at 17:35, Menno Holtkamp @.***> wrote:

Thanks @ilaydaozdemir99 https://github.com/ilaydaozdemir99, merged it and tried to build the project, however it seems I am running into some problems due to outdated packages, such as https://stackoverflow.com/questions/55921442/how-to-fix-referenceerror-primordials-is-not-defined-in-node-js

@jcubic https://github.com/jcubic do you have any up-to-date knowledge on how to get the project "building" again with modern versions of NPM and Gulp?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/longbill/jquery-date-range-picker/pull/514#issuecomment-991919237, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOAUJJ7NHJ2JUTXDEUUADDUQS6LVANCNFSM5J3KQ7WQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

Stamate Cosmin

jcubic commented 2 years ago

I didn't see gulp project for a while. In fact I've never used Gulp.

monovertex commented 2 years ago

This PR is not correct, as the Javascript syntax is not correct. @holtkamp please avoid merging PRs without at least checking if the change is correct in syntax first.

I'm going to revert this merge for the time being, and handle this update later today.

jcubic commented 2 years ago

Maybe it's worth adding some kind of linter and CI using GitHub actions. You will know before merging that there are syntax errors in code.

holtkamp commented 2 years ago

https://github.com/longbill/jquery-date-range-picker/pull/514#issuecomment-992381912 indeed, for PHP I know how to do this. For JavaScript a lot is "magic" for me.

@monovertex you are right, sorry for this, I thought: quick merge and release. Good thing the release failed 😮

jcubic commented 2 years ago

I can add PR with, GitHub actions and ESLint, but I'm not sure if I will be able since you use Gulp. But if you're fine in adding npm scripts in package.json and ignore gulp file that is not needed at all, I can make it work.

jcubic commented 2 years ago

It's seems there is gulp-eslint it may not be that hard.

monovertex commented 2 years ago

This is actually what I first tried to do when started contributing to this repository. However, it is not that easy because of how the library is written. There are a lot of issues in the eslint output, which either required big changes in the code, or turning off many recommended rules.

So in my todo list I had a bigger emphasis on first writing a test suite, and then incorporating some arhitectural changes:

Along with these changes, eslint could then be integrated easily.

ilaydaozdemir99 commented 2 years ago

I'm sorry for my fault @holtkamp @monovertex It was my first PR and I couldn't think I was doing something wrong.