In D7 there are three available/possible links next to each module in the list and each link has an icon. These are the 'Help', 'Permissions' & 'Configure' links (please see attached screenshot). Code review should be such a link with an icon to its left for consistency. This should be done in D7, but it would also be nice to have in D6.

Also, according to drupal standards, commands and links to them should capitalize only the first letter of the first word, so it should be 'Code review' instead of 'Code Review'. Minor thing, I know, that's why I chose to stick it here instead of creating a new issue just for that ;)

Comments

klonos’s picture

StatusFileSize
new6.23 KB

...forgot to attach screenie.

klonos’s picture

Issue tags: +do not capitalize name

ping?

+adding another tag so it's easier to locate similar issues.

klonos’s picture

Title: The '(Code Review)' link should be D7 style (at least for the 7.x version) + It should be 'Code review' instead of 'Code Review' » The '(Code Review)' link should be rendered with the extra links + It should be 'Code review' instead of 'Code Review'

Here's a mockup of how it could look like in the D7 bartik theme.

klonos’s picture

...ping?

I am aware that there might be more important issues at hand, but can I at least get some thoughts on this one? Do you people like it? Doesn't it make sense UI-wise?

stella’s picture

It makes sense but it's just lower priority at the moment. If anyone wants to provide a patch then that might get it in faster.

klonos’s picture

Priority: Normal » Minor

Fair enough Stella.

I must insist though ...now that this really neat way to tidy up the module's page is in progress, it would be a real shame of us to be adding to the mess others are trying hard to deal with: #538904: D8UX: Redesign Modules Page

stella’s picture

I'm happy to, but provide a patch if you want to see it happen sooner.

solotandem’s picture

Status: Active » Needs review
StatusFileSize
new1.97 KB

Adding a link can not be accomplished without a core patch or a patch to your site's administrative theme. As discussed in #913712: Improve modules page table poor layout., while enabling more links, it would make sense to stack the links, as this patch to core does.

The attached patch for this module, along with the core patch above, would make this happen. Unless and until core was patched, you could copy the patched core theme function, theme_system_modules_fieldset(), to your administrative theme. It would look like:

function mytheme_system_modules_fieldset($variables) {
  $form = $variables['form'];

  // Individual table headers.
  $rows = array();
  // Iterate through all the modules, which are
  // children of this fieldset.
  foreach (element_children($form) as $key) {
    // Stick it into $module for easier accessing.
    $module = $form[$key];
    $row = array();
    unset($module['enable']['#title']);
    $row[] = array('class' => array('checkbox'), 'data' => drupal_render($module['enable']));
    $label = '<label';
    if (isset($module['enable']['#id'])) {
      $label .= ' for="' . $module['enable']['#id'] . '"';
    }
    $row[] = $label . '><strong>' . drupal_render($module['name']) . '</strong></label>';
    $row[] = drupal_render($module['version']);
    // Add the description, along with any modules it requires.
    $description = drupal_render($module['description']);
    if ($module['#requires']) {
      $description .= '<div class="admin-requirements">' . t('Requires: !module-list', array('!module-list' => implode(', ', $module['#requires']))) . '</div>';
    }
    if ($module['#required_by']) {
      $description .= '<div class="admin-requirements">' . t('Required by: !module-list', array('!module-list' => implode(', ', $module['#required_by']))) . '</div>';
    }
    $row[] = array('data' => $description, 'class' => array('description'));
    // Display operation links (such as help or permissions) in one column.
    $row[] = array('data' => drupal_render($module['links']), 'class' => array('operations'));
    $rows[] = $row;
  }

  return theme('table', array('header' => $form['#header'], 'rows' => $rows));
}
douggreen’s picture

Status: Needs review » Needs work

I spent an hour on this, and agree it's difficult to do now. The only way I could get the link in the right column is if we change the #theme from system_modules_fieldset to something like coder_modules_fieldset, and then completely re-implement the theme function in coder. This seems too hacky for me, so leaving it as-is until core solves the problem.

I did change references to "Code Review" to "Code review".

klonos’s picture

Thanx for taking the time to update this Doug.

douggreen’s picture

Status: Needs work » Closed (fixed)

Done in 7.x-2.x but apparently at least one person doesn't like it, see #1775954: "Code Review" operations on the module administration page

salvis’s picture

The problem with the Operations table is that it doesn't scale. Every additional column requires a fixed large amount of horizontal space (English is typically quite short, other languages are considerably longer!) and we end up with a large fixed matrix like the Permissions page. The only way to deal with that pattern is to buy a larger monitor, which is not a good solution.

Operations is actually worse than Permissions, because Permissions at least uses every cell, while operations require space even when most cells in the column are empty. And the Modules page first squishes the Descriptions column so that the rows become unbearably high before a scrollbar appears.

klonos’s picture

I completely understand the concerns about the hspace, but the '(Code Review)' link *is* an operation and it belongs in the 'Operations' column. This issue here was a first step at fixing things and should under no circumstance be reverted. I'm afraid we don't see the whole picture here and that's why these concerns have been raised. So, FYI there's talk about:

1. Replacing the operation links with drop-buttons that only display the most-frequently-used operation by default (#1480854: Convert operation links to '#type' => 'operations' & #1608878: Add CTools dropbutton to core). So one operation will be shown at any time - the other ones will be available when one clicks on the drop-down arrow of these buttons. This will make the 'Operations' column more compact.

So something like this:

screenshot of taxonomy vocabulary listing

...would become like this:

The main views listing, showing a dropbutton for 'edit'

...and this:

 edit, disable, clone, export

2. Replacing the operation links with only-icon-no text links in narrow/small screens (#1757368-2: Reduce visual clutter in Modules page or #538904: D8UX: Redesign Modules Page):

The same effect could be optional for wide screens too:

3. Replacing the separate, dedicated cells for each operation link with a single cell so they can be listed vertically in a single cell (#913712-9: Improve modules page table poor layout.). So this:

...would become:

I realize that most of these are D8-only things/features and perhaps they'll never be back-ported to D7, but still -as I previously said- this was a step towards doing things the proper way. Besides, even if they are not backported, that's why we have contrib modules for. Module filter NG already does #3 above (merge links in a single cell under the 'Operations' column) while Module Filter does somewhat the same, but instead removes the whole 'Operations' column altogether and moves links to a separate, single div as a <ul> set to display: inline;.

@salvis: does this info satisfy your concerns perhaps Hans?

salvis’s picture

Thank you for the extensive explanation, klonos!

These are nice UI features and yes, I'll be happy with them. If Coder were a production module, I'd say it ought to wait until these features are available (Don't buy that Hummer before widening your driveway! :)), but it's a development tool and it doesn't need to be enabled unless you're actually using it.

It still is a pain for developers who are confined to a less than huge screen for whatever reasons, but progress wins over considerateness, I guess.

klonos’s picture

No problem ;)