Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Update amMoment.changeLocale() #273

Open
geroyche opened this issue Nov 3, 2016 · 3 comments
Open

Update amMoment.changeLocale() #273

geroyche opened this issue Nov 3, 2016 · 3 comments

Comments

@geroyche
Copy link

geroyche commented Nov 3, 2016

as per the current moment.js docs, moment.locale() is deprecated (and produces warnings in the console). it should be replaced with updateLocale()

moment/moment#3043 (comment)

@urish
Copy link
Owner

urish commented Nov 3, 2016

It seems like only the second parameter to locale was deprecated (i.e. customizing strings). Can you please send a PR that will call updateLocale if the second parameter is provided?

Thanks!

@geroyche
Copy link
Author

geroyche commented Nov 3, 2016

not only so. it's also about whether the locale already exists or not.

.locale(locale) to simply load an existing locale
.locale(locale, opt) to load a newly created one
.updateLocale(locale, opt) to load an existing locale and customize

maybe it would be best to additionally provide .locale() and updateLocale() to your directive? that way existing code will continue to work, and in new code users can decide what to call, based on their knowledge about whether a locale exists already or not.

@urish
Copy link
Owner

urish commented Nov 4, 2016

Yes, that makes sense. I think that the best solution would be to deprecate the second parameter to changeLocale. If you want to define a new locale, or update an existing one - just go directly to moment API.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants