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

feat: add support for lowercase localizable formats (l, ll, lll, llll) #546

Merged
merged 11 commits into from
Apr 1, 2019

Conversation

mariomc
Copy link
Contributor

@mariomc mariomc commented Mar 28, 2019

This PR tries to bridge the gap between dayjs and moment by implementing lowecase/short localizable formats, already present in moment, since 2.0.0

@iamkun
Copy link
Owner

iamkun commented Mar 28, 2019

Thanks, will consider this PR.

@mariomc
Copy link
Contributor Author

mariomc commented Mar 28, 2019

Thanks!
If you wish to keep the localizableFormat plugin slim, I can write another plugin just for this.

@codecov-io
Copy link

codecov-io commented Mar 28, 2019

Codecov Report

Merging #546 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #546   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files       151    152    +1     
  Lines       942    954   +12     
  Branches    123    127    +4     
===================================
+ Hits        942    954   +12
Impacted Files Coverage Δ
src/locale/ko.js 100% <ø> (ø) ⬆️
src/locale/sv.js 100% <ø> (ø) ⬆️
src/locale/lt.js 100% <ø> (ø) ⬆️
src/locale/zh-tw.js 100% <ø> (ø) ⬆️
src/locale/cs.js 100% <ø> (ø) ⬆️
src/locale/ca.js 100% <ø> (ø) ⬆️
src/locale/ja.js 100% <ø> (ø) ⬆️
src/index.js 100% <ø> (ø) ⬆️
src/locale/fi.js 100% <ø> (ø) ⬆️
src/locale/zh-cn.js 100% <ø> (ø) ⬆️
... and 5 more

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 71e4bff...8c6e1b8. Read the comment docs.

@iamkun
Copy link
Owner

iamkun commented Mar 28, 2019

It's not about the size of this plugin, but the size of all other locale files.

If we introduce some more localizable formats, all the locale files will be updated and added these format. That's a huge increase in the total size of our locale files.

That's why I'd like to keep this PR pending until more feature requests.

@mariomc
Copy link
Contributor Author

mariomc commented Mar 28, 2019

I see your worry. However, in the case of moment, there's only a few like zh-cn and sv who set (some) of their own short localized formats.
The big majority of locales don't even define short localized formats and fallback to the default behavior/configuration.

dayjs is well designed and this funcionality can be implemented easily with a plugin (and, for my case, I did just that). I just thought that, by having the current locale plugin with this capacity (hence the PR), it could ease the transition for those who, like me, are migrating from moment to dayjs.

In any case, thank you for the reply and awesome work on dayjs!

@iamkun
Copy link
Owner

iamkun commented Mar 28, 2019

You are right, only a few have this. Is there a full list of locales that have lowercase localizable formats instead of checking every locale files on GitHub one by one?

I'm willing to merge this if it's not that much. 😬

@mariomc
Copy link
Contributor Author

mariomc commented Mar 28, 2019

On moment/locale, doing a grep -e ' l :' -e ' ll : ' -e ' lll :' -e ' llll :' *.js | awk '{print $1}' | sed "s/.js://" | uniq -ci outputs the locale list that use a short-hand localised formats, with the number of different formats each of those use (l, ll, lll, llll):

   3 ca
   1 cs
   4 eu
   4 fi
   4 he
   4 ja
   4 ko
   4 lt
   2 sv
   4 vi
   4 zh-cn
   4 zh-hk
   4 zh-tw

It's a bit more than 10% (13 locales out of 127 total), but still not too many. That should represent roughly 46 new lines total.

@iamkun
Copy link
Owner

iamkun commented Mar 29, 2019

Great!

Skipped locales (fi, he, lt) without (yet) a current format setting in dayjs
@ghost
Copy link

ghost commented Mar 29, 2019

We should update the docs accordinglly

@mariomc
Copy link
Contributor Author

mariomc commented Mar 29, 2019

Will do. Adding current locales as well as fixing current tests to contain those key additions. Noticed an issue with localizedFormats. Will keep in touch and open accordingly.

@mariomc
Copy link
Contributor Author

mariomc commented Mar 29, 2019

Added fix. Will review and add the missing localized formats for fi, he, lt.
Didn't add them previously because the interpolation issue.

@ghost
Copy link

ghost commented Mar 29, 2019

/(\[[^\]]+])| looks nice compare to the original version in core index.js format method.

Maybe we could update the index.js file to make it size smaller?

@mariomc
Copy link
Contributor Author

mariomc commented Mar 29, 2019

Well thought @xxyuk, thanks! We managed to bring the package size down a notch 😄
Should we consider adding final bundle size metrics alongside codecov?

@ghost
Copy link

ghost commented Mar 29, 2019

Not sure, can codecov do this?

@mariomc
Copy link
Contributor Author

mariomc commented Mar 29, 2019

I don't know about codecov. The only one I do know is bundlesize and it looks great!

@ghost
Copy link

ghost commented Mar 29, 2019

Oh I see, we already have such size limit tool during our CI build. You could find them in package.json.

@ghost
Copy link

ghost commented Mar 29, 2019

Maybe update doc here as well ?
https://github.com/iamkun/dayjs/blob/dev/docs/en/Plugin.md#localizedformat

@mariomc
Copy link
Contributor Author

mariomc commented Mar 29, 2019

Done. I also took a dabble at the regex previously mentioned and managed to shave a few more bytes off of it.
Thanks for the heads-up @xxyuk !

src/constant.js Outdated Show resolved Hide resolved
added tests for to cover .format() interpolation
test/parse.test.js Outdated Show resolved Hide resolved
@iamkun iamkun merged commit f2b5ebf into iamkun:dev Apr 1, 2019
iamkun pushed a commit that referenced this pull request Apr 2, 2019
## [1.8.12](v1.8.11...v1.8.12) (2019-04-02)

### Bug Fixes

* Add .get API ([7318797](7318797))
* Add 79 locales ([#541](#541)) ([f75a125](f75a125))
* Add Calendar plugin ([d1b9cf9](d1b9cf9))
* Add isoWeeksInYear plugin ([2db8631](2db8631))
* Add Occitan (oc-lnc) locale file ([#551](#551)) ([c30b715](c30b715))
* Add plugin minMax to sopport .max .min ([2870a23](2870a23))
* Fix set Month Year error in last day of the month ([d058f4a](d058f4a))
* Update ko locale weekdaysShort  ([#543](#543)) ([317fd3e](317fd3e))
* Update localizedFormat plugin to support lowercase localizable formats (l, ll, lll, llll) ([#546](#546)) ([f2b5ebf](f2b5ebf))
@iamkun
Copy link
Owner

iamkun commented Apr 2, 2019

🎉 This PR is included in version 1.8.12 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iamkun iamkun added the released label Apr 2, 2019
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.8.12](iamkun/dayjs@v1.8.11...v1.8.12) (2019-04-02)

### Bug Fixes

* Add .get API ([7318797](iamkun/dayjs@7318797))
* Add 79 locales ([#541](iamkun/dayjs#541)) ([f75a125](iamkun/dayjs@f75a125))
* Add Calendar plugin ([d1b9cf9](iamkun/dayjs@d1b9cf9))
* Add isoWeeksInYear plugin ([2db8631](iamkun/dayjs@2db8631))
* Add Occitan (oc-lnc) locale file ([#551](iamkun/dayjs#551)) ([c30b715](iamkun/dayjs@c30b715))
* Add plugin minMax to sopport .max .min ([2870a23](iamkun/dayjs@2870a23))
* Fix set Month Year error in last day of the month ([d058f4a](iamkun/dayjs@d058f4a))
* Update ko locale weekdaysShort  ([#543](iamkun/dayjs#543)) ([317fd3e](iamkun/dayjs@317fd3e))
* Update localizedFormat plugin to support lowercase localizable formats (l, ll, lll, llll) ([#546](iamkun/dayjs#546)) ([f2b5ebf](iamkun/dayjs@f2b5ebf))
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.8.12](iamkun/dayjs@v1.8.11...v1.8.12) (2019-04-02)

### Bug Fixes

* Add .get API ([7318797](iamkun/dayjs@7318797))
* Add 79 locales ([#541](iamkun/dayjs#541)) ([f75a125](iamkun/dayjs@f75a125))
* Add Calendar plugin ([d1b9cf9](iamkun/dayjs@d1b9cf9))
* Add isoWeeksInYear plugin ([2db8631](iamkun/dayjs@2db8631))
* Add Occitan (oc-lnc) locale file ([#551](iamkun/dayjs#551)) ([c30b715](iamkun/dayjs@c30b715))
* Add plugin minMax to sopport .max .min ([2870a23](iamkun/dayjs@2870a23))
* Fix set Month Year error in last day of the month ([d058f4a](iamkun/dayjs@d058f4a))
* Update ko locale weekdaysShort  ([#543](iamkun/dayjs#543)) ([317fd3e](iamkun/dayjs@317fd3e))
* Update localizedFormat plugin to support lowercase localizable formats (l, ll, lll, llll) ([#546](iamkun/dayjs#546)) ([f2b5ebf](iamkun/dayjs@f2b5ebf))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants