[feature] Calendar function handles formats only arg #3666
Conversation
Corrected the formatting on calendarjs unit test file Corrected autoformatting
|
This looks good to me |
src/lib/moment/calendar.js
Outdated
| @@ -14,6 +15,12 @@ export function getCalendarFormat(myMoment, now) { | |||
| } | |||
|
|
|||
| export function calendar (time, formats) { | |||
| // #3658 - Adding overload to the calendar function in order to allow: | |||
There was a problem hiding this comment.
You can say in a comment above the if along the line of accept format only, but don't include the PR/issue number, there is git blame for that.
|
Got a commit in this morning, cleaned up that comment. |
|
Merged in a2df2a3 |
ichernev
changed the title
[feature] Calendar function handles formats only arg
|
@josephjaniga the code doesn't take into account that To implement this properly (if at all) you need to do key-testing to figure out if its moment-input or formats object, and also allow for functions. Also avoid using typeof whenever possible, we have a bunch of utility functions that try to do a better job. I have to revert this if these are not addressed, otherwise we make the API much worse and edge-casey. |
|
I can take a look at this and get it sorted, looking through the docs for those utility functions you mentioned. |
|
Well, it just needs to work in all cases, not some of them. The current code doesn't work if formats is a function, or you pass an object for moment construction as sole first arg (in which case it will be mis-interpret as a format). http://momentjs.com/docs/#/parsing/object/ |
|
@josephjaniga - thanks for the contribution. In order for us to merge this, you have to address @ichernev 's changes. |
|
Sorry I've been on quite a bit of a hiatus. I am going to bring this one back from the dead. I've re-read everything relevant from the past year, merged down the missed commits, checked the other issues and PRs to make sure this is still a desired feature. I've added some utilities to help checking for input types and have all of the use cases working. Just finishing up on a few final unit tests. Fixing to add about 10 more to the suite. Will update here once that is done. I didn't see anything about updating the docs - can someone point me in the right direction? |
…endar functionality to properly identify time and formats inputs. added a bunch of unit tests
|
Finished my tests, triple checked to make sure I didn't include any new ES6 habits in there and made sure everything is passing locally. Pushed up to CI for final judgement. |
|
@marwahaha Looks like everything passed. 😈 Let me know how it looks and I'll look to see what to do about those docs |
|
Just following up, let me know if there is anything I can do to help keep this moving |
Found the issue ticket #3658, thought it looked like a nice place to start for my first PR on GitHub. Made the recommended changes to overload the calendar function and added a unit test to the suite to cover my use case.