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

Adding support for L10n #12

Open
Wulfheart opened this issue May 8, 2020 · 21 comments
Open

Adding support for L10n #12

Wulfheart opened this issue May 8, 2020 · 21 comments
Labels
enhancement New feature or request

Comments

@Wulfheart
Copy link
Contributor

Wulfheart commented May 8, 2020

I noticed that everything is hardcoded text. I'm going to open a pull request with proper auth texts soon.

@Wulfheart
Copy link
Contributor Author

There are now two pull requests. 🤦‍♂️
#14 and #15.
They differ in the handling of the auth file. #15 is more structured while #14 has a different approach. @danharrin please take the approach you consider more 'laravel-like'.
grafik

@danharrin
Copy link
Member

Hey, we've just been discussing internally the best way to approach this.

I prefer translation keys like you used in #15, but Liam makes a good point that it's not very beginner friendly for a boilerplate, and he would prefer a fix like #14.

Let's see what the community thinks is the best option.

@imliam
Copy link
Contributor

imliam commented May 8, 2020

@pktharindu FYI

I absolutely prefer using keys in a real application that needs full translations, especially combined with a tool like i18n-ally to see the real values inline with code lens, which mitigates the issue of not seeing the real string somewhat.

However, I don't think that it's a very friendly way for most people, especially as we can't force tooling like that, as the majority of apps don't even need internationalisation support. The simpler approach of using the fill string is indeed less flexible, but in my opinion it's so much easier for people to understand exactly what it is, which I think is important in a preset.

What're people's thoughts on this?

@Wulfheart
Copy link
Contributor Author

@imliam In my opinion it should become clear when you read the english strings.
However is it a possibility to add both as a option? As far as I know the php artisan ui does support options.

@pktharindu
Copy link
Contributor

@Wulfheart, I'm really sorry I didn't notice this issue sooner. 😬🙏

I too prefer using translation keys for apps that need full translations, but IMO most of the apps won't need it. Adding translation keys like this adds an overhead of a translation file and makes it a bit harder to understand for beginners. But I guess ultimately it is up to the authors and community to decide.

Again, sorry about not noticing this sooner!

@Wulfheart
Copy link
Contributor Author

No problem @pktharindu. This way I could post a meme. 🙈

@imliam
Copy link
Contributor

imliam commented May 8, 2020

However is it a possibility to add both as a option? As far as I know the php artisan ui does support options.

@Wulfheart I'm not sure how we could do that cleanly without just outright duplicating all the views, any ideas?

@Wulfheart
Copy link
Contributor Author

@imliam Is it possible to run some calculations when running the php artisan ui-command? If yes the corresponding key could be compiled just then.

@danharrin
Copy link
Member

That would be my preferred option.

By default, have the key-based translation strings in the views.

We then have a flag in the UI command that is able to swap these out for the translated versions.

Thoughts @Wulfheart @imliam?

@Wulfheart
Copy link
Contributor Author

Like it. 👍🏻
Do you know if such a “compile” option exists?

@imliam
Copy link
Contributor

imliam commented May 8, 2020

I'd prefer it the other way around myself (the full strings by default because it's easier for people to understand, swapping out to the keys for people that actually want to translate stuff.

We can do whatever custom logic is needed - see the src directory. The service provider can register whatever extra flags it needs to call any custom logic. Right now the most we're doing is using str_replace() on a couple of files, but should be possible to replace all these strings in a loop I guess

@Wulfheart
Copy link
Contributor Author

In this case a lookup-array should be sufficient. The only caveat I can see is that this array has to stay up to date with the lang files.

@danharrin
Copy link
Member

Can we not array_merge and write to the existing lang files instead of overwriting them?

@Wulfheart
Copy link
Contributor Author

@danharrin I'm not sure if this is possible as the language files itself are a php array and not an external json file.

@danharrin
Copy link
Member

@Wulfheart you can require() it which will return the array I'm pretty sure?

@ryangjchandler ryangjchandler added the enhancement New feature or request label May 8, 2020
@pktharindu
Copy link
Contributor

To do this, we'll have to map the translated strings to translation keys or vice versa. So any modification to the translations will require changes in two files, right?

I'm not sure whether it is worth adding that kind of complexity just to achieve the same thing in different ways? 🤷🏼‍♂️

@danharrin
Copy link
Member

@pktharindu we could do this by using the keyed version in the view stubs, and then replacing it with the version stored in the lang file when it's copied over if the user requests. That way there's only one source of truth on our end for translations.

@dakira
Copy link

dakira commented May 18, 2020

I prefer the approach in #14. This is also what other presets use (see the Laravel default presets and the Tailwind presets).

So I'd argue if you go with the keyed version, it wouldn't be Laravel-like, would it? :) It also wouldn't be in line with the rest of the presets in this namespace, so for consistencies sake all of those would have to be changed to keyed translations as well.

@danharrin
Copy link
Member

@dakira we'll probably end up distributing the non-keyed version by default, and then supplying a flag to switch to the keyed version.

@danharrin
Copy link
Member

We want this preset to lead the way with flexibility like that.

@baoanhng
Copy link

baoanhng commented May 24, 2020

I don't get why we need to add complexity or even flexibility to just several authentication blades.
Personally, I don't think a user, who is able to find this repository, needs a friendly help. So I support traditional Laravel-way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants