Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Status: Needs review » Needs work

Looks good already. I have a small suggestion for improvement. Instead of:

However, for links enclosed in translatable texts you should use t('Your admin page', array('@url' => url('admin'))) style to keep strings in context for translation.

I think the following would be clearer:

However, for links enclosed in translatable texts you should use t() and embed the HTML anchor tag directly in the translated string. For example:
@code
t('Your admin page', array('@url' => url('admin')));
@endcode
This keeps the context of the link title ('admin' in the example) translators.

I.e. I think it would be nice to actually say what the best practice in this case is, and then give an example. As a added bonus, api.drupal.org will automatically link "t()" to the respective page.

jhodgdon’s picture

And I would also note that "texts" should be "text" in "enclosed in translatable text". :)

amontero’s picture

Status: Needs work » Needs review
FileSize
1.1 KB

Much clearer that way, for sure. Rock'n'reroll.

amontero’s picture

Doh, example link was missing.

tstoeckler’s picture

Status: Needs review » Needs work

Almost there. Two minor oversights:

+++ b/core/includes/common.inc
@@ -2268,6 +2268,12 @@ function drupal_http_header_attributes(array $attributes = array()) {
+ * t('Your admin page', array('@url' => url('admin')));

This is missing the <a href="@url"> part in the example code.

+++ b/core/includes/common.inc
@@ -2268,6 +2268,12 @@ function drupal_http_header_attributes(array $attributes = array()) {
+ * This keeps the context of the link title ('admin' in the example) translators.

... FOR translators. (missing "for"; this might mean you'll have to wrap the line, don't know)

amontero’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
jhodgdon’s picture

If we want to be really nice, we would also say "administrative" and not "admin" in the text of the example. :)

jhodgdon’s picture

Status: Needs review » Needs work

Actually, can we make a more realistic example from core?

amontero’s picture

IMO, the example should be left as is, because the simpler the example, the better. I doubt that a core example can be more understandable at all and that's precisely the point of the example. So keeping it as short/simple as possible is a plus for me.

jhodgdon’s picture

Well, can it at least be a bit more realistic? I can't imagine anyone actually writing "Your admin page" in text. Maybe something like "Visit the [link]Mymodule settings page[endlink]" instead?

amontero’s picture

How about "Visit the settings page" ?
It keeps it short and without wrapping and it's a more realistic example, as you suggested.

jhodgdon’s picture

Sure!

amontero’s picture

Title: Possible improvement to l() function docs » Improvement to l() function docs
Status: Needs work » Needs review
FileSize
1.13 KB
jhodgdon’s picture

Status: Needs review » Needs work

That looks good, thanks! The only thing (sorry for not noticing it earlier) is that the new text should start on the previous line (all docs comments should wrap as close to 80 characters per line as possible without going over). Or if you think it should be a new paragraph, leave a blank (*) line.

amontero’s picture

amontero’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

That looks great. Thanks for sticking with this!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Documentation, -documentation bug

Thanks! Committed to 8.x and 7.x. I also removed some extraneous tags from this issue (please read the issue tag guidelines! the link is under the Tags field in the help/description text).

xjm’s picture

amontero’s picture

Version: 8.x-dev » 6.x-dev
Status: Fixed » Needs review
Issue tags: +Needs backport to D6
FileSize
1.04 KB

As a comment-only modification, looks both safe and worthwile improving also 6.x docs.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Straight backport. Looks good. :-)

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Good idea -- committed this to 6.x.

Automatically closed -- issue fixed for 2 weeks with no activity.