Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

select2 add i18n for other locale #3598

Merged
merged 5 commits into from
Apr 26, 2021

Conversation

shiroamada
Copy link
Contributor

for select2 blade file, I added language parameter when initial the select2 element.

@if (app()->getLocale() !== 'en')
    language: "{{ str_replace('_', '-', app()->getLocale()); }}",
@endif

image

Added for language support

change to single line and better js view display
during initial obj, the language parameter is need.
Comment on lines 77 to 79
@if (app()->getLocale() !== 'en')
language: "{{ $str_replace('_', '-', app()->getLocale()); }}",
@endif
Copy link
Contributor

Choose a reason for hiding this comment

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

@shiroamada is this part necessary?
Because until now we didn't use it, by only loading the script of the language .../i18n/pt.js it would load the language.
Can you confirm this?

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, it is necessary for zh_CN, i tried, if I didn't add the line, it will not load it.

This is did not add the language: parameter.
image

This is added the language: parameter.
image

Maybe you can check your pt.js is it working when drop down?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @shiroamada, you were right, without that it doesn't work. Meanwhile I moved it to element attributes.
Thank you for you help 🙌

@promatik promatik self-assigned this Mar 6, 2021
@promatik promatik added the Bug label Mar 6, 2021
@tabacitu
Copy link
Member

Hmm this is interesting @shiroamada .

Maybe there's room here to clear up which format we should use for the language string. It's always been one of those questions in Laravel that I didn't know the answer to. And I think you can tell by our lang folder - seems like we're a bit all over the place now, trying to cover all the bases 🙄 I've double-checked and I see Backpack provides:

  • both da-DK and da_DK
  • both fr-CA and fr_CA
  • both pt-BR and pt_BR
  • zh-cn
  • zh-Hant

Also, the only relevant standards I could find are RFC 5646 and [https://tools.ietf.org/html/rfc4646](RFC 4646), both of which encourage dash (not underscore). PHP also gives the same thing as an example.

So I guess the question becomes: why did you choose to make your language zh_CN and not zh-CN? I'm genuinely asking, since I rarely work with dash-languages. Are other important packages using zh_CN and you're stuck with it or something?

The reason I'm asking is that... since the JS library already uses RFC 5646, if you'd use zh-CN... this PR wouldn't be needed any more, right?

Thanks for the PR - really appreciate it. Just trying to get a handle of it since I don't work with dash-languages very often.
Cheers!

@pxpm
Copy link
Contributor

pxpm commented Apr 16, 2021

Hello @shiroamada , thanks for the PR. 👍

@tabacitu this is something we are already aware, more info at: #3537

We should fix this in next version, or in this one using aliases ?

@shiroamada
Copy link
Contributor Author

shiroamada commented Apr 17, 2021

Thanks for your reply. I didn't notice that there is a RFC standard on it.
I was follow by https://github.com/Laravel-Lang/lang/tree/master/locales/zh_CN. I already search people comment about it.
Laravel-Lang/lang#1073

Another popular Laravel-Lang, I believe is created by Chinese, because of the readme also given Chinese explanation.
https://github.com/overtrue/laravel-lang

I went to China Laravel website, they also have people discuss about it.
https://learnku.com/laravel/t/5933/should-the-chinese-language-package-be-zh-cn-or-zh-cn

Another part is I use for underscore(_), because of I use Carbon datime format.

\Carbon\Carbon::setLocale('zh_CN.utf8');
App::setLocale('zh_CN'); //this is where I set the local, follow linux format.
//set date
setlocale(LC_TIME, 'zh_CN.utf8', 'zh_CN');

I also no idea which one is the correct solution. Something new to me.

This one I think should be putting in debate :) Or the best Backpack can support both~

@tabacitu
Copy link
Member

Wow - thanks for the detailed explanation @shiroamada . I had no idea things were so divided. Using str_replace() in this case seems like a sensible solution, thanks 🙏 We should probably assume people are using pt_BR then (but be ready for both).

@pxpm I believe we can support both in 4.1 already, with copy-pasted folders. It'll be duplicate code, but hey... at least we help those people... Then in Backpack 4.2 we can delete the pt-br-like directories and stick to ISO 15897 (pt_BR) if that's what both Laravel and the most popular Laravel language package recommend. Even if we don't agree, it'll be easier for our users to just stick to the standard.

@promatik promatik assigned pxpm and unassigned promatik Apr 24, 2021
@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@promatik
Copy link
Contributor

@tabacitu in my opinion this PR is ready 👌
Since select2 uses kebab-case (pt-BR, en-US, ...) we can make sure the locale is always in that format by replacing _ with -.

Meanwhile, you've closed #3537, and that is another task, a BC for 4.2, clean those duplicated locale folders.
So I'll open #3537, and leave there a note.

@tabacitu tabacitu merged commit 973e8ab into Laravel-Backpack:master Apr 26, 2021
@welcome
Copy link

welcome bot commented Apr 26, 2021

WHOOP-WHOOP! Congrats, your first PR on this repo has officialy been merged.

party

You should also receive an email inviting you to the Community Members team. That's where we, commited community members, debate new features and decide what's in the Backpack roadmap. Feel free to ignore the invitation if you're not interested :-)

If you want to help out the community in other ways, you can:

  • give your opinion on other Github Issues & PRs;
  • chat with others in the Gitter Chatroom (usually for quick help: How do I do X);
  • answer Backpack questions on Stackoverflow; you get points, people get help; you can subscribe to the backpack-for-laravel tag by adding a new filter; that will send you emails when new questions come up with our tag;

Again. Thank you for the PR. You are a wonderful person. Keep 'em coming :-)
Cheers!

--
Justin Case
The Backpack Robot

P.S. Help in the Backpack community is rewarded with free Backpack commercial licenses. It's the least we can do. If you feel you've helped the community with PRs, help & other stuff, please apply for free licenses and mention this PR. You scratch my back, I scratch your back. Thank you!

@tabacitu
Copy link
Member

Great! Thanks again @shiroamada - merged, will be tagged and available with a composer update later today.

@promatik - note to self. In the future we should consider creating helpers to get the app language in these two formats. Something like... hyphened_app_locale() and underscored_app_locale() maybe?!

@promatik
Copy link
Contributor

@tabacitu I agree with that if we have more cases like select2, where we need the app locale "hyphenated".
Let's keep this in mind, and if we ever need the hyphened_app_locale() again, we proceed to the helper ✌

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants