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

Greek language import #514

Merged
merged 2 commits into from
Dec 12, 2021
Merged

Greek language import #514

merged 2 commits into from
Dec 12, 2021

Conversation

ilaydaozdemir99
Copy link
Contributor

#511 Greek language has been added.

Greek language has been added.
extra line has been deleted
@ilaydaozdemir99 ilaydaozdemir99 changed the title Update jquery.daterangepicker.js Greek language import Dec 11, 2021
@holtkamp holtkamp merged commit a140bff into longbill:master Dec 12, 2021
@holtkamp
Copy link
Collaborator

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
Copy link
Collaborator

monovertex commented Dec 12, 2021 via email

@jcubic
Copy link

jcubic commented Dec 12, 2021

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

@monovertex
Copy link
Collaborator

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.

monovertex added a commit that referenced this pull request Dec 13, 2021
monovertex added a commit that referenced this pull request Dec 13, 2021
@jcubic
Copy link

jcubic commented Dec 13, 2021

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
Copy link
Collaborator

#514 (comment) 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
Copy link

jcubic commented Dec 13, 2021

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
Copy link

jcubic commented Dec 13, 2021

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

@monovertex
Copy link
Collaborator

monovertex commented Dec 13, 2021

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:

  • Breaking down the library in several files
  • Using modern ES syntax + babel
  • Migrating from gulp to webpack
  • And of course, upgrading to the latest npm/node versions

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

@ilaydaozdemir99 ilaydaozdemir99 deleted the patch-1 branch December 15, 2021 20:39
@ilaydaozdemir99
Copy link
Contributor Author

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

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.

4 participants