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: fix it locale error #458

Merged
merged 4 commits into from Feb 2, 2019
Merged

fix: fix it locale error #458

merged 4 commits into from Feb 2, 2019

Conversation

iamkun
Copy link
Owner

@iamkun iamkun commented Jan 18, 2019

@anbi can you review this pr please?

@codecov-io
Copy link

codecov-io commented Jan 18, 2019

Codecov Report

❗ No coverage uploaded for pull request base (dev@97a6088). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##             dev   #458   +/-   ##
====================================
  Coverage       ?   100%           
====================================
  Files          ?     54           
  Lines          ?    473           
  Branches       ?     72           
====================================
  Hits           ?    473           
  Misses         ?      0           
  Partials       ?      0
Impacted Files Coverage Δ
src/locale/es-do.js 100% <ø> (ø)
src/locale/it.js 100% <ø> (ø)
src/locale/es.js 100% <ø> (ø)
src/locale/es-us.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97a6088...2474436. Read the comment docs.

@bi-a
Copy link

bi-a commented Jan 18, 2019

Done.

iamkun referenced this pull request Jan 19, 2019
* DE locale: shot months

* ES locale: shot months

* IT locale: shot months

* UK locale: short months, short and min weekdays

* FR locale: short months

* German short months start with upper-case letter
@iamkun
Copy link
Owner Author

iamkun commented Jan 19, 2019

@naulacambra Just updated locale/es.js according to momentjs. Can you review please.
If it's ok, I'll update the test file accordingly.

Plus, after compare to momentjs locale file, there are es.js, es-do.js and es-us.js. But Dayjs has a es-es.js locale file. Is it something wrong?

@naulacambra
Copy link
Contributor

@naulacambra Just updated locale/es.js according to momentjs. Can you review please.
If it's ok, I'll update the test file accordingly.

It's ok 😀

Plus, after compare to momentjs locale file, there are es.js, es-do.js and es-us.js. But Dayjs has a es-es.js locale file. Is it something wrong?

I'll try to explain it correclty. Spanish is spoken worldwildly, but there are some little differences between some regions. es-es is the Spain's spanish (from where am I), es-us United States's spanish, and es-do is the Dominican Republic's spanish. While there are some differences in the vocabulary, the name of months or weekdays, are the same, so for this reason there is no need to have one file for each region.

We should decide which standard we use. Language nomenclature (es for all Spanish regions) or language-region nomenclature (es-es, es-us,...)

@iamkun
Copy link
Owner Author

iamkun commented Jan 21, 2019

Oh I see.
Not sure if I understood you correctly

It seems es-es is equal to es (Castilian Spanish), and es-us, es-do... has some different.

So that es-es.js is the same with es.js in dayjs, and we should just delete es-es and consider adding es-us, es-do like moment does?

@iamkun
Copy link
Owner Author

iamkun commented Jan 26, 2019

@naulacambra any suggestions here for my comment above? thanks #458 (comment)

@naulacambra
Copy link
Contributor

@iamkun sorry for the delay.

In this context (dates, week days, etc) es-es, es-us, or es-do are the same. Even they have words with different meaning (they have different words for the same thing), they are the same language and share most of their words.

Regarding your opinion #458 I would delete es-es.js file keeping only the es.js, but I don't believe we'll need to add es-do or es-us files, since they would be the same (check moment localization files, they all are the same)

@iamkun
Copy link
Owner Author

iamkun commented Jan 28, 2019

@naulacambra thanks.

According to your replay, at present, we should just delete es-es.

And if we add some more cases in locales that might have different between es and es-us, we could add es-do or es-us at that time accordingly.

Right?

@naulacambra
Copy link
Contributor

@iamkun exactly 😀

@iamkun
Copy link
Owner Author

iamkun commented Jan 28, 2019

Finally I decided to add es-do and es-us.
So that can keep the same with moment, on the other hand, this could also increase our total locale count. 😬

@naulacambra
Copy link
Contributor

So that can keep the same with moment, on the other hand,

Ok, makes sense.

this could also increase our total locale count.

Since they need to be loaded by the user, I don't increase the "default" size, so I think we're fine with that 😀

@iamkun iamkun merged commit 689e167 into dev Feb 2, 2019
@iamkun iamkun deleted the feature/iamkun_fix_it_locale branch February 2, 2019 04:21
iamkun added a commit that referenced this pull request Feb 2, 2019
* fix: fix it locale error

* fix: fix es locale monthsShort error

* test: fix test

* fix: add missing es-do and es-us locale file
iamkun pushed a commit that referenced this pull request Feb 2, 2019
## [1.8.2](v1.8.1...v1.8.2) (2019-02-02)

### Bug Fixes

*  Add missing czech language locale ([#461](#461)) ([7e04004](7e04004))
* add deltaZone in the equation when calculating diff and added utcOffset api method ([#453](#453)) ([ce2e30e](ce2e30e))
* fix it locale error ([#458](#458)) ([f6d9a64](f6d9a64))
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.8.2](iamkun/dayjs@v1.8.1...v1.8.2) (2019-02-02)

### Bug Fixes

*  Add missing czech language locale ([#461](iamkun/dayjs#461)) ([7e04004](iamkun/dayjs@7e04004))
* add deltaZone in the equation when calculating diff and added utcOffset api method ([#453](iamkun/dayjs#453)) ([ce2e30e](iamkun/dayjs@ce2e30e))
* fix it locale error ([#458](iamkun/dayjs#458)) ([f6d9a64](iamkun/dayjs@f6d9a64))
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.8.2](iamkun/dayjs@v1.8.1...v1.8.2) (2019-02-02)

### Bug Fixes

*  Add missing czech language locale ([#461](iamkun/dayjs#461)) ([7e04004](iamkun/dayjs@7e04004))
* add deltaZone in the equation when calculating diff and added utcOffset api method ([#453](iamkun/dayjs#453)) ([ce2e30e](iamkun/dayjs@ce2e30e))
* fix it locale error ([#458](iamkun/dayjs#458)) ([f6d9a64](iamkun/dayjs@f6d9a64))
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.

None yet

4 participants