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

Fixed the meridiem value for Chinese. #222

Closed
wants to merge 2 commits into from

Conversation

rockymeza
Copy link
Contributor

In Chinese, there are more values for the meridiem value than just AM and PM. There are values that mean morning, before noon, noon, afternoon, and night time. I had to modify moment.js to handle a callback or default to the previous behavior.

If you accept #221, I am pretty sure that you will have a merge conflict in the calendar day tests. Here is what those tests should look like:

// zh-cn.js
test.equal(moment(a).calendar(),                     "今天早上2点00",     "today at the same time");
test.equal(moment(a).add({ m: 25 }).calendar(),      "今天早上2点25",     "Now plus 25 min");
test.equal(moment(a).add({ h: 1 }).calendar(),       "今天早上3点00",     "Now plus 1 hour");
test.equal(moment(a).add({ d: 1 }).calendar(),       "明天早上2点00",     "tomorrow at the same time");
test.equal(moment(a).subtract({ h: 1 }).calendar(),  "今天早上1点00",     "Now minus 1 hour");
test.equal(moment(a).subtract({ d: 1 }).calendar(),  "昨天早上2点00",     "yesterday at the same time");
// zh-tw.js
test.equal(moment(a).calendar(),                     "今天早上2點00",     "today at the same time");
test.equal(moment(a).add({ m: 25 }).calendar(),      "今天早上2點25",     "Now plus 25 min");
test.equal(moment(a).add({ h: 1 }).calendar(),       "今天早上3點00",     "Now plus 1 hour");
test.equal(moment(a).add({ d: 1 }).calendar(),       "明天早上2點00",     "tomorrow at the same time");
test.equal(moment(a).subtract({ h: 1 }).calendar(),  "今天早上1點00",     "Now minus 1 hour");
test.equal(moment(a).subtract({ d: 1 }).calendar(),  "昨天早上2點00",     "yesterday at the same time");

In Chinese, there are more values for the meridiem value than just AM
and PM.  There are values that mean morning, before noon, noon,
afternoon, and night time.  I had to modify moment.js to handle a
callback or default to the previous behavior.
@timrwood
Copy link
Member

Hmm, if we're adding moment.meridiem.lower and moment.meridiem.upper, should we just deprecate moment.meridiem.am/AM/pm/PM and default to hard coded am/pm?

I dont think any other languages use moment.meridiem.am besides south-east asian languages.

@rockymeza
Copy link
Contributor Author

Are you saying that in all of the other language files, am/AM/pm/PM are all the same?

@timrwood
Copy link
Member

Yes, am/pm are the same for all languages but Korean and Chinese.

Meridiem was added for the Korean translation, and I didn't know if any future language would need different strings for upper and lower case, hence the semi-redundant am/AM, pm/PM.

@rockymeza
Copy link
Contributor Author

Is it possible to unify the meridiem method into one method? There is no point in having two methods if the only languages that would even use it don't even have uppercase/lowercase.

We could delete meridiem am/AM/pm/PM in all other language files and define a meridiem function that accepts hour, minute, case in those few languages that are different. That would shrink the filesize of most of the language definitions a little bit.

@timrwood
Copy link
Member

Yeah, that's probably even better.

@rockymeza
Copy link
Contributor Author

I went ahead and did that, but you don't have to merge that part of the pull request in if you don't want to.

I had to change something in the way that languages are added to moment that perhaps changed some functionality. I don't know if it is documented or intentional, but there is the capability for a language definition to inherit from another language definition. Previously, the language definition would inherit from the last language defined. Now it inherits from English. I don't know if this functionality was intentional. When I removed all of the meridiem references it broke the tests with regard to the languages after Korean.

Since English is default and is always available I decided that language definition files could inherit from it instead of disabling inheritance altogether.

@rockymeza
Copy link
Contributor Author

The tests pass if we take off inheritance or if we use the English as a fallback. However, I don't know about people's custom language definition files, if indeed there are any.

Thoughts?

@timrwood
Copy link
Member

Yeah, there's no way to know about people's own language files, but I think we can document the upgrade path and what was depreciated from 1.5 to 1.6.

I think inheriting from english is a good idea. It gives the idea of a solid base, unlike how it currently acts, where it depends on the previous language. It will certainly shrink down this file too.

https://github.com/timrwood/moment/blob/master/lang/en-gb.js

timrwood added a commit that referenced this pull request Mar 23, 2012
Changing to a function.
#222 #228
@timrwood
Copy link
Member

The pull request got botched, so I updated everything manually. Can you double check that the Chinese calendar day tests are correct?

b34ba94#L20R150
b34ba94#L21R150

@rockymeza
Copy link
Contributor Author

Looks good to me. Do the tests still pass?

-rocky
On Mar 24, 2012 2:45 AM, "Tim Wood" <
reply@reply.github.com>
wrote:

The pull request got botched, so I updated everything manually. Can you
double check that the Chinese calendar day tests are correct?

b34ba94#L20R150

b34ba94#L21R150


Reply to this email directly or view it on GitHub:
#222 (comment)

@timrwood
Copy link
Member

Yeah, they pass, but I kinda went about it backwards, copy/pasting the results of the test back into the unit tests themselves. If it all looks good to you, I'll close this out.

@timrwood timrwood closed this Mar 26, 2012
@rockymeza
Copy link
Contributor Author

fine by me

@jimmyye
Copy link

jimmyye commented Jan 16, 2013

There is a bug in the meridiem function.

            meridiem : function (hour, minute, isLower) {
                if (hour < 9) {
                    return "早上";
                } else if (hour < 11 && minute < 30) {
                    return "上午";
                } else if (hour < 13 && minute < 30) {
                    return "中午";
                } else if (hour < 18) {
                    return "下午";
                } else {
                    return "晚上";
                }
            },

When 11:31 am, it should be "中午" (noon), but it becomes "下午" (afternoon).
&& minute < 30 is wrong and unnecessary because noon means 11:00 to 13:00 in China.
This should work:

            meridiem : function (hour, minute, isLower) {
                if (hour < 9) {
                    return "早上";
                } else if (hour < 11) {
                    return "上午";
                } else if (hour < 13) {
                    return "中午";
                } else if (hour < 18) {
                    return "下午";
                } else {
                    return "晚上";
                }
            },

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.

3 participants