Support from Acquia helps fund testing for Drupal Acquia logo

Comments

floydm’s picture

Status: Active » Needs review
FileSize
1.18 KB
thedavidmeister’s picture

Status: Needs review » Needs work
-  $output = theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'shortcuts'), 'empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array('@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link')))));
+
+  $table = array(
+    '#theme' => 'table',
+    '#header' => $header,
+    '#rows' => $rows,
+    '#attributes' => array(
+        'id' => 'shortcuts',
+        'empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array('@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link')))
+        )
+    );
+
+  $output = drupal_render($table);

Only indent new lines 2 spaces instead of 4 please.

danylevskyi’s picture

danylevskyi’s picture

Issue tags: +CodeSprintUA
danylevskyi’s picture

Assigned: danylevskyi » Unassigned
Status: Needs work » Needs review
FileSize
1.17 KB
podarok’s picture

Status: Needs review » Reviewed & tested by the community

#5 if bot happy - rtbc
look good for me

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3c420a6 and pushed to 8.x. Thanks!

tstoeckler’s picture

Status: Fixed » Needs work
+++ b/core/modules/shortcut/shortcut.admin.inc
@@ -283,7 +283,18 @@ function theme_shortcut_set_customize($variables) {
+      'empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array('@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link')))

This should be a top-level #empty instead, as can be seen in theme_table(). You can verify that this is currently broken when visiting admin/config/user-interface/shortcut/manage/default. Before the patch the empty message displayed, now it doesn't. Instead you will find an "empty" attribute on the < table > element.

thedavidmeister’s picture

Category: task » bug

dang

drupee’s picture

Assigned: Unassigned » drupee
Status: Needs work » Needs review
FileSize
829 bytes

Good Point! I just fixed it.

drupee’s picture

#10: empty_message_fixed-2009668.patch queued for re-testing.

BarisW’s picture

According to the coding standards, shouldn't this be written like this:

<?php
'#empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array(
  '@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link'),
)),
?>
drupee’s picture

I refered this ,

"However, for links enclosed in translatable text you should use t() and embed the HTML anchor tag directly in the translated string. For example:

t('Visit the settings page', array('@url' => url('admin')));
This keeps the context of the link title ('settings' in the example) for translators. "

at https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/l/8

can somebody please recheck.

BarisW’s picture

I meant the start of the values in the array on the next line for arrays spanning over 80 characters.

<?php
  '#empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array(
  '@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link'),
)),
?>
drupee’s picture

Okay, thanks for explanation. I have updated the patch likewise. Please check now.

BarisW’s picture

Status: Needs review » Needs work

Sorry for being such an arse, but the closing brackets should be on a new line:

instead of:

<?php
'#empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array(
'@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link'))),
?>

It should read (mark the comma after url(),):

<?php
'#empty' => t('No shortcuts available. <a href="@link">Add a shortcut</a>.', array(
  '@link' => url('admin/config/user-interface/shortcut/manage/' . $form['#shortcut_set_name'] . '/add-link'),
)),
?>
drupee’s picture

Can you recheck now?

drupee’s picture

Status: Needs work » Needs review
BarisW’s picture

And now you're missing two spaces ;)

This should nail it.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f7d8998 and pushed to 8.x. Thanks!

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