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

Added deltaZone in the equation when calculating diff #452

Closed
wants to merge 1 commit into from

Conversation

naulacambra
Copy link
Contributor

I've looked into momentjs code, and tried to understand why method dayjs().diff() was returning different values than moment().diff() when the unit was day. Happens that momentjs uses the offset of each element to calculate hour zone difference, and adds it to the equation.

I've had to add the utcOffset() method in the core (where the diff() method is), but only the get version.

This should resolve #384

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #452 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #452   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          52     52           
  Lines         467    469    +2     
  Branches       72     72           
=====================================
+ Hits          467    469    +2
Impacted Files Coverage Δ
src/constant.js 100% <ø> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97a6088...c9ca021. Read the comment docs.

@iamkun
Copy link
Owner

iamkun commented Jan 15, 2019

Oh I see, nice catch.

Besides, I think we could add utcOffset as a public api to our user.

Docs and test should be included.

How do you think? Or I could make a pr if I got some time.

@iamkun iamkun added the next release Will merge into next release label Jan 15, 2019
@iamkun
Copy link
Owner

iamkun commented Jan 15, 2019

PS, could you please re-open a new pr to merge your code to our dev branch.

It might good to have a dev branch waiting for next release.

@naulacambra
Copy link
Contributor Author

Besides, I think we could add utcOffset as a public api to our user.

Sure, I'll add the utcOffset documentation and tests

PS, could you please re-open a new pr to merge your code to our dev branch.

Ok, i'll close this PR and open a new one

@iamkun
Copy link
Owner

iamkun commented Jan 16, 2019

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release Will merge into next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants