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 greek grammar for Saturday on formatRelative #1930

Merged
merged 4 commits into from
Aug 31, 2020
Merged

Fixed greek grammar for Saturday on formatRelative #1930

merged 4 commits into from
Aug 31, 2020

Conversation

BanForFun
Copy link
Contributor

No description provided.

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

I appreciate the change in formatRelative but you did unrelated change and also forgot to commit the snapshot (run yarn locale-snapshots and commit the result)

@@ -4,7 +4,7 @@ var dateFormats = {
full: 'EEEE, d MMMM y',
long: 'd MMMM y',
medium: 'd MMM y',
short: 'd/M/yy'
short: 'd/M/y'
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this change is not correct. y does not provide shorter result:

> dateFns.format(new Date(), 'd/M/y')
'31/8/2020'
> dateFns.format(new Date(), 'd/M/yy')
'31/8/20'

I know it's confusing, but it's needed for dates like 5 AD:

> const date = new Date()
undefined
> date.setFullYear(5)
-61988407055185
> dateFns.format(date, 'd/M/y')
'31/8/5'
> dateFns.format(date, 'd/M/yy')
'31/8/05'

Copy link
Contributor Author

@BanForFun BanForFun Aug 31, 2020

Choose a reason for hiding this comment

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

Sorry, about that. I just committed the locale snapshot. The change in formatRelative was intentional, but I forgot to mention it. I didn't want for the year component to become shorter, I just think that the full year format is more readable (and more commonly used in Greece).

Copy link
Member

Choose a reason for hiding this comment

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

Please rollback the change and use a custom format in your code. d/M/yy corresponds to the standard: https://unicode-org.github.io/cldr-staging/charts/37/summary/el.html#57dac0d1b36c1261

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know that, but I can assure you that the vast majority of people in Greece write the year as four digits. Do you want me change it to yyyy or roll it back to yy?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I can't rely on subjective feelings. It's already challenging to support locales as I can only speak two languages, but date-fns supports dozens. Without a standard to consult, this will turn into chaos.

If you are 100% sure that the standard is incorrect, I will be happy to reconsider, but we'll have to do it in a separate pull-request with a review from other native speakers.

Please revert to yy.

I'm about to ship a new version, and if you can make this change and commit once again updated snapshot right now, it will go live right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -4,7 +4,7 @@ var dateFormats = {
full: 'EEEE, d MMMM y',
long: 'd MMMM y',
medium: 'd MMM y',
short: 'd/M/yy'
short: 'd/M/y'
Copy link
Member

Choose a reason for hiding this comment

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

Please rollback the change and use a custom format in your code. d/M/yy corresponds to the standard: https://unicode-org.github.io/cldr-staging/charts/37/summary/el.html#57dac0d1b36c1261

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Please rollback the change and use a custom format in your code. d/M/yy corresponds to the standard: https://unicode-org.github.io/cldr-staging/charts/37/summary/el.html#57dac0d1b36c1261

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Thank you a lot!

@kossnocorp kossnocorp merged commit 242e4e3 into date-fns:master Aug 31, 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

2 participants