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

[l10n] Add Basque (eu) locale and improve Spanish (es-ES) locale #10819

Merged
merged 12 commits into from
Nov 10, 2023
Merged

[l10n] Add Basque (eu) locale and improve Spanish (es-ES) locale #10819

merged 12 commits into from
Nov 10, 2023

Conversation

lajtomekadimon
Copy link
Contributor

@lajtomekadimon lajtomekadimon commented Oct 27, 2023

Add Basque l10n for date pickers.

@lajtomekadimon lajtomekadimon marked this pull request as draft October 27, 2023 08:30
@LukasTy LukasTy added l10n localization component: pickers This is the name of the generic UI component, not the React module! labels Oct 27, 2023
@mui-bot
Copy link

mui-bot commented Oct 27, 2023

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running yarn l10n
  • Verify that you have added an export line in src/locales/index.ts for the new locale.
  • Run yarn docs:api which should add your new translation to the list of exported interfaces.
  • Clean files with yarn prettier.

Deploy preview: https://deploy-preview-10819--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 58ce7f6

@lajtomekadimon lajtomekadimon marked this pull request as ready for review October 27, 2023 08:33
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! 🙏
It looks like that we did not have this locale in the existing locales map, hence, it does not end up included in the table of available locales.
Could you please add this locale entry in the scripts/localeNames file? 🤔
P.S. Please run yarn l10n after doing it in order to generate the updated localization table statistics. 👌

@lajtomekadimon
Copy link
Contributor Author

Thank you for your contribution! 🙏 It looks like that we did not have this locale in the existing locales map, hence, it does not end up included in the table of available locales. Could you please add this locale entry in the scripts/localeNames file? 🤔 P.S. Please run yarn l10n after doing it in order to generate the updated localization table statistics. 👌

Done!

packages/x-date-pickers/src/locales/euES.ts Outdated Show resolved Hide resolved
@lajtomekadimon
Copy link
Contributor Author

Spanish and Basque translations fixed and completed! 🥳

@lajtomekadimon lajtomekadimon changed the title [l10n] Add Basque (eu-ES) locale to date pickers [l10n] Fix Spanish (es-ES) locale and add Basque (eu-ES) locale to date pickers Oct 27, 2023
@LukasTy LukasTy changed the title [l10n] Fix Spanish (es-ES) locale and add Basque (eu-ES) locale to date pickers [l10n] Add Basque (eu-ES) locale and improve Spanish (es-ES) locale Oct 27, 2023
@lajtomekadimon
Copy link
Contributor Author

Is there anything else to do so you can merge this PR?

@LukasTy
Copy link
Member

LukasTy commented Nov 3, 2023

Is there anything else to do so you can merge this PR?

Hey, thank you once again for your contribution! 🙏
Sorry to not notify you about this, but we are in a bit of a discussion regarding the localization policy: mui/material-ui#39525.
There is nothing more you need to do. 👌

@michelengelen michelengelen changed the base branch from master to next November 6, 2023 14:18
@@ -5,6 +5,7 @@ export * from './deDE';
export * from './elGR';
export * from './enUS';
export * from './esES';
export * from './euES';
Copy link
Member

@oliviertassinari oliviertassinari Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we shorten it? e.g. Chocobozzz/PeerTube#3956

Suggested change
export * from './euES';
export * from './eu';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. That's a very good point and valuable discussion
@lajtomekadimon Could you confirm your position on this topic? Do you agree that having only eu as a locale name makes sense?

Copy link
Contributor Author

@lajtomekadimon lajtomekadimon Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both "eu" and "eu-ES" are fine as identifiers for the language, but I don't think eu alone is a good idea in our case. euES is the right choice here, mainly because that's the same criteria applied for the other languages.

All locale codes of packages/x-date-pickers/src/locales/ have both the language code and the country code. You have enUS instead of en for English, esES instead of es for Spanish, etc. Even languages like Icelandic or Japanese, which are only spoken on one country (at least to my knowledge), are isIS and jaJP.

I can't see the reason why Basque must be an exception and have a neutral country code.

And if that is not enough reason, keep in mind that a locale represents regional settings. It's much more than a language: it includes how numbers, dates and times are display, among other things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your points and they do make sense.
However, have you read the linked discussion? There are some compelling reasons and examples as to why only using eu for this and some other locales makes sense. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've read it. But that makes sense only if you apply the same criteria to all languages.

I'm not sure if changing the policy of all locale codes is related to this PR. I think that should be addressed on its own issue, since that affects all languages, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first look, it doesn't seem like a rare occasion. There is probably around at least a third of the locales that can have their country specific suffix removed. 🙈

@LukasTy I wonder if there are this many, e.g. ja yes, en no, fr no, es no, de no.

Copy link
Member

@LukasTy LukasTy Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliviertassinari If we follow the path that DateFns, Dayjs and Moment went, then all your mentioned examples are actually yes, because they went with fr for French (French), but add the country specific suffix for different locales.
It would especially make sense if we go the "inheritance"/"alias" route, where frCH implementation would be:

import { fr } from './fr';

export const frCH = {
  ...fr,
  theDifferentKey: 'theDifferentValue',
};

WDYT? 🤔

Copy link
Member

@oliviertassinari oliviertassinari Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT? 🤔

@LukasTy Alright, here is my clearer take on it. The root of the problem here is this:

basque-mapS

a. If we use eu_ES and eu_FR it would be confusing because these are the same
b. If we only use eu_ES, it could be seen as unfair for people living in France
c. If we only use eu, it could be seen as an exception to the rule

Overall, I think c. is the way, under the rule that we precise locales as much as possible but without being too precise when it could lead to confusion. This case is rare.


Then there are other cases:

  1. Languages that only exist in one locale, e.g. ja-JP. But https://github.com/iamkun/dayjs/blob/dev/src/locale/ja.js feels wrong, being more specific is clearer.
  2. Languages in different locales, e.g. en-UK, en-US. But https://github.com/iamkun/dayjs/blob/dev/src/locale/en.js feels wrong. It's not specific enough, I want to know exactly what en is. I wouldn't use this. I think that the only value of en is to help people who are short in time, who don't have the bandwidth to support too many locales, to know what's the best approximation (most popular locale).

My #suggestion would go like this:

  • 👍 to merge this PR
  • 👍 to create en as an alias of en-US
  • 👍 when we create en-UK, to have it as an extension of en-US
  • 👍 to not touch es-ES, fr-FR, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. Languages that only exist in one locale, e.g. ja-JP. But https://github.com/iamkun/dayjs/blob/dev/src/locale/ja.js feels wrong, being more specific is clearer.

I agree about the clarity aspect, but I feel that's what the libraries have addressed by adding a comment about the exact locale specifics, like in this case: https://github.com/date-fns/date-fns/blob/main/src/locale/pt-BR/index.ts#L11
I feel that having the comments would benefit our localization files even in this state to further increase the clarity. 🤔
This comment ties into the 3rd point as well.

Copy link
Member

@oliviertassinari oliviertassinari Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

@LukasTy I think that the problem is that this is not clear from the codebase of developers. They have to open the docs or look at the IntelliSense.

@LukasTy LukasTy changed the title [l10n] Add Basque (eu-ES) locale and improve Spanish (es-ES) locale [l10n] Add Basque (eu) locale and improve Spanish (es-ES) locale Nov 9, 2023
@LukasTy
Copy link
Member

LukasTy commented Nov 10, 2023

I'm merging this. 👌
Thank you for your contribution and eternal patience while we were discussing the best solution @lajtomekadimon! 🙏

@LukasTy LukasTy merged commit c565302 into mui:next Nov 10, 2023
18 checks passed
@flaviendelangle flaviendelangle mentioned this pull request Nov 10, 2023
15 tasks
LukasTy added a commit to LukasTy/mui-x that referenced this pull request Nov 10, 2023
…#10819)

Co-authored-by: David Andrés <dandres@binovo.es>
Co-authored-by: Lukas <llukas.tyla@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! l10n localization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants