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

Add moment-hijri to plugins #164

Merged
merged 2 commits into from
Jul 31, 2015
Merged

Add moment-hijri to plugins #164

merged 2 commits into from
Jul 31, 2015

Conversation

xsoh
Copy link
Contributor

@xsoh xsoh commented Dec 3, 2014

Can you please add moment-hijri plugin description to plugins section

@xsoh
Copy link
Contributor Author

xsoh commented Feb 1, 2015

Is there a problem with the request?

@mattjohnsonpint
Copy link
Contributor

Hi @xsoh - I just stumbled upon this PR. Sorry it has been sitting for so long!

I have a concern about your plugin. You use the h specifier to indicate a Hijri date, but moment already uses h to represent the non-padded hour component of a 12-hour time.

For example, in your readme you show:

moment().format('hYYYY/hM/hD');

Without your plugin loaded, moment will return "32015/36/318" because it's currently 2015-06-18 at 3:00 PM.

I find it a bit concerning to change the output of a function just based on whether or not another library is loaded. The user may get incorrect numbers just based on a loading error. It would be better to use a token that is not in use already, perhaps i for Islamic or q for the q in Umm al-Qura?

@xsoh
Copy link
Contributor Author

xsoh commented Jun 19, 2015

@mj1856 alright, we'll discuss that.
thanks

@mattjohnsonpint
Copy link
Contributor

Thanks! I'll leave this open for now then.

@xsoh
Copy link
Contributor Author

xsoh commented Jul 31, 2015

@mj1856 The specifier has been updated to i.
If there is anything else please tell me and I'll do it straightaway 😄 👍

mattjohnsonpint added a commit that referenced this pull request Jul 31, 2015
Add moment-hijri to plugins
@mattjohnsonpint mattjohnsonpint merged commit 2a45d00 into moment:master Jul 31, 2015
@mattjohnsonpint
Copy link
Contributor

Sorry for the delay. Merged.

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