Skip to content

Conversation

sarhugo
Copy link

@sarhugo sarhugo commented Apr 29, 2014

Check needed mods to use directly with momentjs and without datejs and custom helpers.
Maybe a possible branch?

@jockmac22 jockmac22 added this to the v0.2b release milestone May 15, 2014
@jockmac22
Copy link
Owner

sarhugo,

I've reviewed your code, and I like the solution, however I've opted not to merge the branch because of a few design concerns.

Your code base removes a group of functions around line 34 (e.g. fcdp.getDateFromString, fcdp.moveMonth, etc.).

While they add code to the base, I have those methods in place as a way of maintaining centralized code within the code base so that if decisions are made to swap out date manipulation libraries (like you did with date and momentum), then you have to touch the entire code base, instead of just the few methods that handle the manipulations.

Also, the change in the date format strings is a much deeper issue that initially presented. I am using the formatting conventions from Ruby with intention and the move to momentum changes that structure. Additionally, this change would force people who upgrade to v0.2b to change all of their format strings (which includes me, and I'm inherently lazy. ;) ).

I will keep this pull request as a reference for the modifications I'm making in v0.2b, because I think a lot of what you have there is good. For instance, I like the 0-padding approach which uses simple numeric comparison and string concatenation, and I'm not completely beyond the notion of moving to momentum, I just need to think about the date formats more before I jump to it.

Thanks for the effort on this.

  • Jocko

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants