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

[locale] br: Add meridiem translation and correct quotemark #5433

Merged
merged 15 commits into from May 20, 2020

Conversation

o-Chamie-o
Copy link
Contributor

eur e brezhoneg Lizherennañ

Mat eo g.m. evit PM (goude merenn) ha a.m. evit AM (a-raok merenn).

--
Should be g.m. for PM, stand for goude merenn and a.m. for AM stand for a-raok Merenn

Here some sources :
http://www.brezhoneg.bzh/177-divizou-hag-erbedadennou-ar-chuzul-skiantel.htm
https://www.unicode.org/cldr/charts/latest/summary/br.html

@coveralls
Copy link

coveralls commented Apr 2, 2020

Coverage Status

Coverage increased (+0.007%) to 88.492% when pulling 7373dbf on o-Chamie-o:patch-2 into 04b275c on moment:develop.

@ichernev
Copy link
Contributor

@jbleduigou can you comment on this please.

@o-Chamie-o it would help if you speak english :)

@jbleduigou
Copy link
Contributor

Hi,

Thanks for contributing!

We actually moved away from the 12 hour notation.
See this PR: #4975

I don't know if it has been actually released?

@o-Chamie-o
Copy link
Contributor Author

o-Chamie-o commented Apr 23, 2020

it would help if you speak english :)

All translated on my comment. :) @ichernev

Hum strange choice, @jbleduigou as I mentioned, the Unicode recommandation and the one from the OPAB are 12 based.

I can double check this maybe ? 🤔

@jbleduigou
Copy link
Contributor

Hum strange choice, @jbleduigou as I mentioned, the Unicode recommandation and the one from the OPAB are 12 based.

I can double check this maybe ? 🤔

I actually don't have strong opinions on this one.
It might make more sense to use 12 hours.

@ichernev
Copy link
Contributor

@o-Chamie-o about the other changes -- they mostly revolve around this quote. Do people use these special characters when they type? If not then parsing will be completely broken (for reading in dates from user input).

@o-Chamie-o
Copy link
Contributor Author

o-Chamie-o commented Apr 24, 2020

I true that, the parsing will be broken. :( As on the keyboard the single quote is used instead of the apostrophe.

Can we add option in the parser ? For both single quote and apostrophe maybe ?

In order to display it right. :)

@o-Chamie-o
Copy link
Contributor Author

This should do the trick right ? @ichernev

var monthsParse = [/^genver/i, /^c[ʼ\']hwevrer/i, /^meurzh/i, /^ebrel/i, /^mae/i, /^mezheven\/i, /^gouere/i, /^eost/i, /^gwengolo/i, /^here/i, /^du/i, /^kerzu/i];
var monthsShortParse = [/^gen/i, /^c[ʼ\']hwe/i, /^meu/i, /^ebr/i, /^mae/i, /^eve\/i, /^gou/i, /^eos/i, /^gwe/i, /^her/i, /^du/i, /^ker/i];
var weekdaysParse = [/^sul/i, /^lun/i, /^meurzh/i, /^merc[ʼ\']her/i, /^yaou/i, /^gwener\/i, /^sadorn/i,];
...
monthsParse : monthsParse,
monthsShortParse : monthsShortParse,
weekdaysParse  : weekdaysParse ,

Other advantage, we now support case incensitive case. ;)

@ichernev
Copy link
Contributor

ichernev commented Apr 27, 2020

@o-Chamie-o well, yes. If we don't want to break existing code the parsing regexes should be more lenient.

Also make sure to add tests for parsing with different quote. Esp important if any names contain spaces (I think this is not the case for this locale, but still).

@jbleduigou
Copy link
Contributor

Hi,
Sorry for the delay, I have been fairly busy lately and didn't get a chance to review this PR.
I went through the links you provided.

Using a.m. and p.m. was definitely wrong.
Switching to 24 hours notation was an okay workaround.
But ultimately I believe that your suggestion with a.m and g.m. is better.

Thanks for contributing!

@o-Chamie-o
Copy link
Contributor Author

o-Chamie-o commented May 6, 2020

No worries @jbleduigou :)

Thanks to you for the BR locale support in the first place. :)

@marwahaha
Copy link
Member

@o-Chamie-o glad to hear you have approval from the locale author.
Could you please update with the latest code on develop branch and resolve conflicts?
If you need help with this let me know.
Then we can merge it!

@o-Chamie-o
Copy link
Contributor Author

Arf some issues with the month parsing. :(

@marwahaha
Copy link
Member

Oh no :( hope you can get it fixed!

@marwahaha marwahaha changed the title [locale] br: Fix lack of meridiem override [locale] br: Add meridiem translation and correct quotemark May 19, 2020
@marwahaha marwahaha removed the todo label May 19, 2020
@marwahaha marwahaha merged commit 315abe8 into moment:develop May 20, 2020
@o-Chamie-o
Copy link
Contributor Author

🙏 Thanks a bunch @marwahaha
Mersi bras dit. :)

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

5 participants