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

Updated Duration documentation #22

Closed
wants to merge 1 commit into from
Closed

Conversation

stebogit
Copy link

@stebogit stebogit commented Oct 1, 2020

Should you merge iamkun/dayjs#1099 😜

@iamkun
Copy link
Member

iamkun commented Oct 4, 2020

I prefer not to add this to the docs. It's a little wired syntax I think. 🤔

@stebogit
Copy link
Author

stebogit commented Oct 4, 2020

You mean the way I phrased it or the actual new option?

@iamkun
Copy link
Member

iamkun commented Oct 4, 2020

I think we should not tell our new user that we could add a duration directly.

And adding this feature is only for migration from moment.js.

@stebogit
Copy link
Author

stebogit commented Oct 4, 2020

But if it is not in the docs, how would people migrating from moment learn this is available?
Is there a "how to migrate from moment" document maybe?

@iamkun
Copy link
Member

iamkun commented Oct 5, 2020

Cause moment support this, so we implement this. But this does not mean we should encourage our users to use this syntax.

I think a clear way for most users is to convert duration to milliseconds by our user and to added to the Dayjs instance.

@iamkun
Copy link
Member

iamkun commented Dec 29, 2020

I'll close this since it's been a while since it's been opened. Feel free to reopen if you have updates on this

@iamkun iamkun closed this Dec 29, 2020
iamkun added a commit that referenced this pull request Dec 29, 2020
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.

2 participants