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

[new locale] nl-be: Dutch (Belgium) locale #3416

Closed
wants to merge 3 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@kevinzwhuang
Contributor

kevinzwhuang commented Sep 2, 2016

These changes address issue #3410:

  • Renames nl locale to nl-nl for Dutch (Netherlands) locale
  • Creates nl-be for Dutch (Belgium) locale

References for nl-be locale:

References for nl-nl locale:

How does this look to you @jorisroling, @middagj?

@middagj

This comment has been minimized.

Contributor

middagj commented Sep 5, 2016

I am Dutch (as in nl-nl), so I can't really comment on the standard Belgian way of writing dates. I do see it sometimes like that, and the linked sources do confirm. Is it possible to have a common ancestor of the two files?

(Sorry about the line comment, only if the month is numeric slashes are used so no need to change the if statement.)

@kevinzwhuang

This comment has been minimized.

Contributor

kevinzwhuang commented Sep 5, 2016

I just added a commit to restore the original src/locale/nl.js & src/test/locale/nl.js to serve as a common ancestor for both nl-nl & nl-be, does that work @middagj?

@middagj

This comment has been minimized.

Contributor

middagj commented Sep 5, 2016

Oh that is also nice, for if you don't care about NL or BE. I was actually more referring to duplication in the code. I don't know if that is easily done in the current framework.

@kevinzwhuang

This comment has been minimized.

Contributor

kevinzwhuang commented Sep 5, 2016

Yeah, duplicating it as NL for now is probably the way to go. AR and the
other AR locales ( AR-LY, AR-MA, ...etc ) also follow this pattern ( see
ar.js, ar-ly.js, etc. )

This is nice too since anyone using NL currently will not suffer a breaking
change from losing the locale to NL-NL.

On Mon, Sep 5, 2016, 11:54 AM Jacob Middag notifications@github.com wrote:

Oh that is also nice, for if you don't care about NL or BE. I was actually
more referring to duplication in the code. I don't know if that is easily
done in the current framework.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#3416 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AG64ch7Qe_rOAsSPpm7TgbQ0Fr32V1cgks5qnGV9gaJpZM4JzdtT
.

@kevinzwhuang

This comment has been minimized.

Contributor

kevinzwhuang commented Sep 14, 2016

Revisiting this PR after a while, @mj1856 how does this look to you?

@mj1856 mj1856 added the Language label Sep 14, 2016

@mj1856

This comment has been minimized.

Member

mj1856 commented Sep 14, 2016

You don't need to do anything with nl, nor do you need a nl-nl copy. Just add the one file nl-be.js (and its tests).

Consider that moment.locale('nl-anything') will currently use nl.

@kevinzwhuang

This comment has been minimized.

Contributor

kevinzwhuang commented Sep 14, 2016

Thanks @mj1856 , I went ahead and removed nl-nl. Now the only addition will be nl-be and its tests

@kevinzwhuang

This comment has been minimized.

Contributor

kevinzwhuang commented Oct 3, 2016

Does this PR look good to you @mj1856 ?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 1, 2016

@kevinzwhuang code looks great. You'd also need to sing the CLA (a new thing). This will go out in next release. Thanks for contributing!

@kevinzwhuang

This comment has been minimized.

Contributor

kevinzwhuang commented Nov 1, 2016

Thanks @ichernev, just signed the CLA!

@kevinzwhuang kevinzwhuang changed the title from Create nl-be (Belgium) locale & rename nl (Netherlands) to nl-nl to Create nl-be (Belgium) locale Nov 1, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 6, 2016

Merged in 0ad3055

@ichernev ichernev changed the title from Create nl-be (Belgium) locale to [new locale] nl-be: Dutch (Belgium) locale Nov 6, 2016

@ichernev ichernev closed this Nov 6, 2016

ichernev added a commit that referenced this pull request Nov 6, 2016

Merge pull request #3416 from kevinzwhuang:migrate-nl-locales
[new locale] nl-be: Dutch (Belgium) locale

@mj1856 mj1856 added this to the 2.16.0 milestone Nov 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment