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

fix: update fa locale #2151

Merged
merged 1 commit into from
Dec 6, 2022
Merged

fix: update fa locale #2151

merged 1 commit into from
Dec 6, 2022

Conversation

Soheil-Saberi
Copy link
Contributor

update translate month ,monthsShort , weekdaysShort and relativeTime.past

@iamkun
Copy link
Owner

iamkun commented Dec 3, 2022

we use https://github.com/moment/moment/blob/develop/src/locale/fa.js as a reference, can you please tell us why such a difference between the two?

@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

Merging #2151 (f0982d9) into dev (cbe91fd) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               dev     #2151   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          183       183           
  Lines         2096      2096           
  Branches       550       549    -1     
=========================================
  Hits          2096      2096           
Impacted Files Coverage Δ
src/locale/fa.js 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Soheil-Saberi
Copy link
Contributor Author

Soheil-Saberi commented Dec 3, 2022 via email

@iamkun
Copy link
Owner

iamkun commented Dec 4, 2022

Thanks for the details above.

I'm afraid I can't decide whether to merge this or not since it has a lot of differences from the original moment's locale.

I'll label this pr and wait for more reviews from your native speakers.

@iamkun iamkun added help wanted Extra attention is needed Need Review labels Dec 4, 2022
@Soheil-Saberi
Copy link
Contributor Author

Thank you for paying attention to this issue
Additionally, I share this issue with Farsi language developers to contribute to its improvement

@cnaez
Copy link

cnaez commented Dec 5, 2022

I am agree with Soheil.
We need some changes to make dayjs better for Iranian developers.
Thank you guys

@iamkun
Copy link
Owner

iamkun commented Dec 6, 2022

Cheers

@iamkun iamkun merged commit 1c26732 into iamkun:dev Dec 6, 2022
iamkun pushed a commit that referenced this pull request Dec 6, 2022
## [1.11.7](v1.11.6...v1.11.7) (2022-12-06)

### Bug Fixes

* Add locale (zh-tw) meridiem ([#2149](#2149)) ([1e9ba76](1e9ba76))
* update fa locale ([#2151](#2151)) ([1c26732](1c26732))
@iamkun
Copy link
Owner

iamkun commented Dec 6, 2022

🎉 This PR is included in version 1.11.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iamkun iamkun added the released label Dec 6, 2022
@raminjafary
Copy link
Contributor

raminjafary commented Dec 7, 2022

@iamkun @Soheil-Saberi This PR is getting it wrong by simply translating locale months to Solar Hijri calendar but not taking care of solar calendar's date and time transformations. Dayjs' locale provides literal translations of the Gregory calendar's months which is correct. I believe this PR should be reverted to avoid confusions and misunderstanding.

I suggest to use this plugin for Persian users. It handles all the date-time logic of solar calendar along with correct names of the months.
https://github.com/alibaba-aero/jalaliday

@Soheil-Saberi
Copy link
Contributor Author

Soheil-Saberi commented Dec 7, 2022

hi @raminjafary
This PR has been posted before, you will understand with a more detailed examination of the dayjs library and the usage of the local file.
By checking the local files of other countries in dayjs and their calendars, you can understand this issue

@raminjafary
Copy link
Contributor

raminjafary commented Dec 7, 2022

Most of the countries do not have solar calendar. As a result, they have one to one mapping of English [literal] translations for the months. But, If you use solar (Persian) months, you have to use solar calendar date-time, too.

@Soheil-Saberi
Copy link
Contributor Author

@raminjafary
Not all countries use the Gregorian calendar, so check the local file of other countries.
Read this link for more information about different calendars.

@raminjafary
Copy link
Contributor

raminjafary commented Dec 7, 2022

There is no guarantee that the other locale files of the other countries in Dayjs repo is being approved context wise.
Dayjs is neither a solar nor lunar system based calendar so it doesn't make sense to provide translations of any other calendar system as is without the necessary date time modifications.
For example, As of my experience, January which is يناير in ar-IQ locale is just a Gregorian equivalent with the same date time system. Although the first day of the week is different, it is configurable via Dayjs locale plugin.

@Soheil-Saberi
Copy link
Contributor Author

@raminjafary
If you check all the methods of the Dayjs library, you will notice the use of the locale method.
All methods in the Dayjs library do not perform date conversions.
These locale files of the Dayjs library are mostly references of other libraries for the use of its translation, for example, in library Mantine for Dates, when the locale library Dayjs is called, only the names of the months are taken(link mantain/dates)
You can see examples of these use cases in other libraries that use Dayjs.

epszaw pushed a commit to epszaw/dayjs that referenced this pull request Feb 18, 2023
## [1.11.7](iamkun/dayjs@v1.11.6...v1.11.7) (2022-12-06)

### Bug Fixes

* Add locale (zh-tw) meridiem ([iamkun#2149](iamkun#2149)) ([1e9ba76](iamkun@1e9ba76))
* update fa locale ([iamkun#2151](iamkun#2151)) ([1c26732](iamkun@1c26732))
jnorris-cs pushed a commit to catalystio/dayjs that referenced this pull request Mar 29, 2023
## [1.11.7](iamkun/dayjs@v1.11.6...v1.11.7) (2022-12-06)

### Bug Fixes

* Add locale (zh-tw) meridiem ([iamkun#2149](iamkun#2149)) ([1e9ba76](iamkun@1e9ba76))
* update fa locale ([iamkun#2151](iamkun#2151)) ([1c26732](iamkun@1c26732))
@amirhmoradi
Copy link

Hi everyone.
I agree with @raminjafary on the fact that when using a calendar system the locale object shall provide the correct names of that calendar system and not another calendar system.
i.e.
if using Gregorian calendar,
-> month names shall be in Gregorian calendar month in all languages:
----> fr: Janvier, en: January, fa: ژانویه

if using Persian calendar (shamsi/jalalli),
-> month names shall be in Persian calendar month in all languages:
----> fr: Ordibehešt, en: Ordibehesht, fa: اردیبهشت

It does not make sense, neither it is useable from a user's use-case point of view, to translate the NAME of first month of the Gregorian calendar to the NAME of first month of another calendar without converting dates.

The #2151 PR is not correct in this matter. A new PR is required to fix the fa locale.

To solve the issue around calendar systems with dayjs, I have created a new plugin that allows using multiple non-gregorian calendars and does correct conversions. Have a look here: https://github.com/calidy-com/dayjs-calendarsystems

Also, my plugin does an override on the fa locale to fix the currently running inconsistency in month names.

@mokhosh
Copy link

mokhosh commented Jul 24, 2023

What an irresponsible PR!
This change is based on a wrong plugin architecture where the OP doesn't try to handle generation of month names in the plugin, rather he changes the original month names regardless of what calendar system dayjs is using.

This needs to be reverted asap.

Related: alibaba-aero/jalaliday#39

@raminjafary
Copy link
Contributor

raminjafary commented Jul 25, 2023

@iamkun FYI

@raminjafary
Copy link
Contributor

I opened a PR to undo these changes. @mokhosh @amirhmoradi It would be great if I could have your comments or confirmations on this PR #2411.

@github-actions
Copy link

🎉 This issue has been resolved in version 1.11.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

ohsory1324 pushed a commit to ohsory1324/dayjs that referenced this pull request Dec 20, 2023
ohsory1324 pushed a commit to ohsory1324/dayjs that referenced this pull request Dec 20, 2023
## [1.11.7](iamkun/dayjs@v1.11.6...v1.11.7) (2022-12-06)

### Bug Fixes

* Add locale (zh-tw) meridiem ([iamkun#2149](iamkun#2149)) ([1e9ba76](iamkun@1e9ba76))
* update fa locale ([iamkun#2151](iamkun#2151)) ([1c26732](iamkun@1c26732))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed Need Review released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants