-
Notifications
You must be signed in to change notification settings - Fork 53
Support splitting up messages.js to locale-based .js files #48
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
Conversation
andywer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Good stuff!
Just left a question about the new @section. Doesn't seem to be immediately related to this PRs primary changes.
Otherwise 👍👍
resources/views/head.blade.php
Outdated
| </script> | ||
| @stop | ||
|
|
||
| @section('js-localization.head.exported') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that new @section actually a part of the file splitting changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, well, kind of. It allows you to use @yield('js-localization.head.exported'), which in turn will include the <script> tags for the exported files if they exist, and fallback to messages.js or just serving from the API endpoint instead if used incorrectly.
README.md
Outdated
| This will in turn _also_ generate the following file(s) in your target directory: | ||
| * `lang-{locale}.js` contains one language's translation strings, if the `split_export_files` config option is set to true | ||
|
|
||
| With static generation you can also make use of `@yield('js-localization.head.exported')` in your blade template. This will automatically include your statically generated .js files for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andywer I briefly mention the new @yield here, which is related to the @section part below
resources/views/head.blade.php
Outdated
| @stop | ||
|
|
||
| @section('js-localization.head.exported') | ||
| @if (file_exists(config('js-localization.storage_path') . '/js-localization.min.js')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite hard to understand: Why do we check for ${storage_path}/js-localization.min.js and then set the URL to url('/vendor/js-localization/js-localization.min.js') / url('/js-localization/localization.js')?
It's not obvious how ${storage_path}/js-localization.min.js and the URLs relate to each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right of course. If someone would change the storage_path config, that would not work indeed. Maybe skip the @section altogether? Otherwise one would have to have a function that returns the correlation of storage_path and public_path (if that exists) to be able to figure out where the exported files actually go. And that might be a little bit out of scope for the plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would probably be the easiest solution now: Removing the section again and changing the Readme to tell the user how to use the locale-specific files.
Could still add a section like this in a follow-up PR.
|
The Thanks for sharing this! |
carestad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed @section from blade. Updated readme.
* Add support for splitting up the messages.js file into language/locale based .js files as well. * Correct indenting in readme * Correct typo in README * Improve README for the static generation part a bit
Seeing as loading the whole messages.js file might be a bit too much, I have added support for splitting up the whole messages.js into .js files for each locale (lang-en.js, lang-es.js etc.).
This way, one could chose to only load the translations for the user's language, by checking
App::getLocale().Also added a
@yield(@yield('js-localization.head.exported')) in the view for this that will check for the exported files and include accordingly.Also updated docs and did some small indent fixes as well.