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

[locale] zh-cn: Improve next/prev week #5447

Merged
merged 2 commits into from May 20, 2020

Conversation

ulion
Copy link
Contributor

@ulion ulion commented Apr 19, 2020

This error was introduced by f38d69c

This PR mostly restore the calendar output to the most common formats, which is almost quite same with what it was before above commit.

This PR passed the test and will fix:
#4097
#4149
#4354
#5414

And you can close other invalid PR like: #5357

@jsf-clabot
Copy link

jsf-clabot commented Apr 19, 2020

CLA assistant check
All committers have signed the CLA.

@ulion ulion changed the title Fix wrong calendar output for zh-cn for same week days. [bugfix] Fix wrong calendar output for zh-cn for same week days. Apr 19, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 88.333% when pulling 2af84df on ulion:feature/locale_calendar_zhcn_fix into 13a61b2 on moment:develop.

@coveralls
Copy link

coveralls commented Apr 19, 2020

Coverage Status

Coverage decreased (-0.0005%) to 88.485% when pulling 915aa66 on ulion:feature/locale_calendar_zhcn_fix into e3baef9 on moment:develop.

@Alanscut
Copy link
Contributor

Alanscut commented Apr 20, 2020

@ulion Do you need to fix zh-hk.js and zh-tw.js simultaneously? This should be a general problem for other locales.

@ulion
Copy link
Contributor Author

ulion commented Apr 20, 2020 via email

@ulion
Copy link
Contributor Author

ulion commented Apr 20, 2020

@ben-lin @hehachris @skfd

@ichernev
Copy link
Contributor

@zenozeng can you comment on that please.

if (now.week() !== this.week()) {
return '[下]dddLT';
} else {
return '[本]dddLT';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to explicitly mention for the current week?

Suggested change
return '[本]dddLT';
return 'dddLT';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is about accurate, when your are in Saturday, you say Sunday, it will be the next week's Sunday, means '下'(next) week, not '本'(this) week
But this nearest rule is not explicated, so you probably don't know which week it is. while on the other hand, if you say Monday of this week (本周一), Sunday of this week(本周日), it's explicated located an accurate day which refer to current day, share the same week, in certain culture (with the Sunday belong to previous week or next week rule setup).

And before the commit mentioned in my PR which resulted wrong calendar output, the code did this too, which is the proper way to make the representation of the day as explicated accurate as possible and simple. Just as you said Monday of next week (下周一),the Monday of this week(本周一) is simply same rule and same clear. while just "Monday" would be unclear if you are in a day which is more close to the next Monday.

@hehachris
Copy link
Contributor

@ulion Do you need to fix zh-hk.js and zh-tw.js simultaneously? This should be a general problem for other locales.

Agree with @Alanscut

@ulion
Copy link
Contributor Author

ulion commented Apr 23, 2020

@ulion Do you need to fix zh-hk.js and zh-tw.js simultaneously? This should be a general problem for other locales.

Agree with @Alanscut

I already mentioned @ben-lin @hehachris @skfd who authored that two locale. I'm not sure the culture of that two region indeed, so can not decide for them. let them help themselves would be best.

@ichernev ichernev changed the title [bugfix] Fix wrong calendar output for zh-cn for same week days. [locale] zh-cn: Improve next/prev week Apr 25, 2020
@marwahaha
Copy link
Member

@ulion Could you update your PR with the latest code?
I am not sure what the status of this is. Did @zenozeng or @suupic approve?

@ulion
Copy link
Contributor Author

ulion commented May 19, 2020

@marwahaha updated.

@marwahaha
Copy link
Member

@ulion You may need to run eslint fix and prettier

./node_modules/eslint/bin/eslint.js Gruntfile.js tasks src --fix
npm run prettier-fmt

@ulion ulion force-pushed the feature/locale_calendar_zhcn_fix branch from 563b898 to b961799 Compare May 19, 2020 13:29
@ulion
Copy link
Contributor Author

ulion commented May 19, 2020 via email

@marwahaha
Copy link
Member

@ulion I am not sure we will get the original locale authors to approve. If you like, please add yourself as a locale author for zh-cn.

@ulion
Copy link
Contributor Author

ulion commented May 19, 2020

This does basically go back to old 'correct' version, which basically came from f4d4efd and the author is still list in the file, just no activity on github these two years.
Since no new things added here, I'm fine to be not listed.
r u there? @zenozeng though probably not I guess.

@marwahaha
Copy link
Member

Ok.
Would any of you like to be locale authors?
I would prefer to add one more who has been on GitHub recently.
@tokinonagare
@awmleer
@willin
@huiop368
@uu109

@uu109
Copy link

uu109 commented May 20, 2020

Thank you for asking, I would love to be a locale author of 'zh-cn' language, hope to help you.

Copy link

@willin willin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marwahaha marwahaha merged commit 4d0f390 into moment:develop May 20, 2020
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.

None yet

9 participants