search

jalaali / jalaali-js

JavaScript functions for converting Jalaali and Gregorian calendar systems to each other
MIT License
310 stars 36 forks source link

Add time params to jalaaliToDateObject method #29

Closed mhsattarian closed 3 years ago

behrang commented 3 years ago

Hey,

Thanks for the PR. However, I see that you've used default parameters, which is a ES2015 feature. Even though most browsers support it, I believe this library is written in the old style (for example uses var instead of const and let). So It's better to keep it that way.

I think maybe just dropping the = null part from the function definition will be enough.

It would be great if you add a few test cases too.

mhsattarian commented 3 years ago

You're right. I fixed the default params in a new commit.

will add the test too in the next one.

mhsattarian commented 3 years ago

I've added tests for jalaaliToDateObject with time params, and run them locally. everything looks fine.

here's a screenshot, just in case:

image

behrang commented 3 years ago

Thanks, merged.