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

Feature/implement generic add #1504

Closed
wants to merge 14 commits into from
Closed

Feature/implement generic add #1504

wants to merge 14 commits into from

Conversation

Anshuman71
Copy link
Contributor

@Anshuman71 Anshuman71 commented Oct 25, 2019

Implement the add(Object) function from Moment and fix #1427

Add 2 hours, 15 minutes and 30 seconds to 10 July 2014 12:45:00;

var result = add(new Date(2014, 6, 10, 12, 45, 0), { minutes: 15, hours: 2, seconds: 30 });

result: Thu Jul 10 2014 15:00:30

Supports adding

{
    hours: number,
    years: number,
    days: number,
    quarters: number,
    weeks: number,
    months: number,
    minutes: number,
    seconds: number
} 

combined in a single call

@Anshuman71
Copy link
Contributor Author

@leshakoss @alaskaa @dkozickis can you please review this PR

Copy link
Contributor

@dkozickis dkozickis left a comment

Choose a reason for hiding this comment

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

Thank you for your PR @Anshuman71!
Please go through the comments.

CHANGELOG.md Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/index.js Outdated Show resolved Hide resolved
src/add/test.js Show resolved Hide resolved
Copy link
Contributor

@leshakoss leshakoss left a comment

Choose a reason for hiding this comment

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

The function is good, but as of now in a bundle it would be huge: it includes, for example, the impossible condition if (arguments.length < 2) ... 8 times.

Please optimize the function by copying and pasting the logic inside all of addX functions into this one, optimizing where possible (for example you can add ms, seconds, minutes and hours on the same line because all of these are milliseconds multiplied by some constant). After that, the function should be able to do everything addX do, but without importing them.

@Anshuman71
Copy link
Contributor Author

The function is good, but as of now in a bundle it would be huge: it includes, for example, the impossible condition if (arguments.length < 2) ... 8 times.

Please optimize the function by copying and pasting the logic inside all of addX functions into this one, optimizing where possible (for example you can add ms, seconds, minutes and hours on the same line because all of these are milliseconds multiplied by some constant). After that, the function should be able to do everything addX do, but without importing them.

Thankyou, I will surely optimize the code with the above advise

@kossnocorp
Copy link
Member

@Anshuman71 hey, I love the PR, but I see that I've removed the repo, and now I can't resolve the conflicts and clean up the code. I'll appreciate it if you can push it again or send me the patch file. Thanks!

@Anshuman71 Anshuman71 closed this Jan 3, 2020
@Anshuman71
Copy link
Contributor Author

Hey @kossnocorp, thanks for your feedback. I moved this to #1581 . Please look into and let me know if need any changes.

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.

Feat: multi add function to add multiple parts of a date
4 participants