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

Autoload app's js translations #13350

Merged
merged 7 commits into from Jan 14, 2015
Merged

Autoload app's js translations #13350

merged 7 commits into from Jan 14, 2015

Conversation

BernhardPosselt
Copy link
Contributor

As discussed on IRC this tries to automatically load js translations for all apps (that is if your app calls OC_Util::addScript('appName', 'file') and not OC_Util::addScript('madnessmagic').

This currently breaks calendar because they exploit a bug in the method to do weird shit like

OC_Util::addScript('calendar/3rdparty/js', 'file') 

instead of

OC_Util::addScript('calendar', '../3rdparty/js/file') 

We can patch this easily though

PS: this also loads translations for apps that register their scripts in app.php which is an antipattern.

@PVince81 your addTranslations method seems broken. It does not seem to add any translations at all unless i add translation('news') to my index.php. Then all translations are added properly. We also should handle non existent translations so that the page does not break if the translation is not present. You need to add a check for this in your addTranslations method and fall back to english if no file is found

@georgehrke @raghunayyar @MorrisJobke @DeepDiver1975

Fixes #12490

@PVince81
Copy link
Contributor

@Raydiation are you talking about OC.addTranslations() from JS or from PHP ?

@BernhardPosselt
Copy link
Contributor Author

@PVince81 since we add script paths ownCloud will break if these scripts are not present. So we should check if they exist

@PVince81
Copy link
Contributor

  • remove debug statements
  • move loadedTranslations check to addTranslations

@PVince81
Copy link
Contributor

@PVince81
Copy link
Contributor

@DeepDiver1975 do we agree on this new approach ?

@PVince81
Copy link
Contributor

@Raydiation thanks.

I'll update the docs.

@PVince81
Copy link
Contributor

Fixes #12490

@PVince81 PVince81 added this to the 8.0-current milestone Jan 14, 2015
@BernhardPosselt
Copy link
Contributor Author

Fix for calendar is here owncloud/calendar#671

@ghost
Copy link

ghost commented Jan 14, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/6758/
👍 Test PASSed. 👍

@DeepDiver1975
Copy link
Member

tested 👍

@DeepDiver1975
Copy link
Member

sweet shit - thx

@PVince81
Copy link
Contributor

It worked for me too, so 👍

@PVince81
Copy link
Contributor

I'd feel even more confident to merge this if another person could check/test this, @MorrisJobke ?

@MorrisJobke
Copy link
Contributor

Works here too 👍

MorrisJobke added a commit that referenced this pull request Jan 14, 2015
@MorrisJobke MorrisJobke merged commit 455ad00 into master Jan 14, 2015
@MorrisJobke MorrisJobke deleted the autoload-translations branch January 14, 2015 17:12
@scrutinizer-notifier
Copy link

The inspection completed: 8 new issues, 1 updated code elements

@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lazy load JS translation fallback for apps that forgot addTranslation
5 participants