? Success

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
9 Oct 2013

Mainly wondering why there is a regex for this message when a simple language string would work as well.

I admit the language string isn't very easy to read with all those escape slashes. But that's the only downside I see so far and I think the language teams are capable of handling this.

I also removed the str_replace thing which is unneeded as long as the translations are done properly.

If we want to keep the actual URLs out of the language string, we could also use JText::sprintf() and pass the URLs as parameters. Personally I don't think we have to expect that those links are changing.

Potential issue: Can the order of the links and button changed for RTL languages (or maybe also others) with the current approach? I don't think they can but I'm not a pro with regex.

Tracker:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32223

avatar Bakual Bakual - open - 9 Oct 2013
avatar beat
beat - comment - 9 Oct 2013

Hi @Bakual,

Thanks for your contribution proposal and your wish to simplify current code (by moving the complexity and lots of responsibility to translators, official ones and non-official ones).

Code-review:

This patch is moving HTML markup and Javascript code into the language fields.

It simplifies already working and tested code a bit (maybe a bit more for not-regex friends), and move the complexity to the language string and translators.

In that string there is not only the "a" links, but also the button too with javascript code in it.

We do not want translators to have handle javascript code and HTML markup. That needs to stay in the component view.

The original string has been shown to translators and approved as easy to translate. The new string is way less easier.

It is going against clean separatation of markup, code and strings, and is really not best practice. Additionally it would allow overrides of the links and the functions by simple language strings, which is not a good idea, technically, marketing-wise and legally.

There is no gain at all from this patch, neither in features, nor performance, nor bug fixes, as it just proposes to change a already working and tested code.

Thus I think that it's not a good proposal and I strongly think that it should not be accepted for merging.

avatar Bakual
Bakual - comment - 9 Oct 2013

Actually, we have a lot of HTML code in the language files. I did a quick search for the opening bracket < and found ~1000 hits in backend and ~350 hits in frontend. So it can't be that wrong imho.

As for the JavaScript code, that's true. But it is only the onclick call (onclick=\"Joomla.submitbuttonInstallWebInstaller()\").

Additionally it would allow overrides of the links and the functions by simple language strings, which is not a good idea, technically, marketing-wise and legally.

There are various links already in the language files, including one to the licence. So I don't think legal or marketing is a problem here.

Any comments on this?

Potential issue: Can the order of the links and button changed for RTL languages (or maybe also others) with the current approach? I don't think they can but I'm not a pro with regex.

Like is it possible to show the button on the left side and change the order of the links? If I understand the current regex correct this is not possible, but I don't have enough experience with RTL languages and Regex to be sure.

avatar beat
beat - comment - 9 Oct 2013

The current implementation in the CMS works fine in RTL, and has been tested in RTL by testers and myself too. Thanks for the concern.

Language strings should be... language strings, and nothing else.

My above comments remain valid, and i don't wish to repeat myself (enough other work that has to be done which makes a difference to the user....).

Thus I still think that it's not a good proposal and I strongly think that it should not be accepted for merging.

avatar Bakual
Bakual - comment - 9 Oct 2013

Changed it to use JText::sprintf() and pass most HTML code as arguments. There is now even less HTML in it than with the original code (<button> is removed as well).

Language string:
COM_INSTALLER_INSTALL_FROM_WEB_INFORMATION="<a %1$s>Joomla! Extensions Directory</a> now available with <a %2$s>Install from Web</a> on this page:&nbsp;&nbsp; %3$s"

It needs one additional language string for the button text:
COM_INSTALLER_INSTALL_FROM_WEB_ADD_TAB="Add &quot;Install from Web&quot; tab"

I think this code is easier to understand and remains easy to translate at the same time.

Add a Comment

Login with GitHub to post a comment