Menu admin page uses inconsistent formatting - should use a table like other editable lists

Thanks for the screenshots. I see how the tabs have been reworded to single-word operations. That's a bit tricky:
- What is the meaning of the "list" operation? I'm guessing "show all menu items" but it's not very obvious.
- Same for 'add'. Again, probably "Add menu item" but easily interpreted to mean "Add menu".

You probably did this to save space, could you add another screenshot of the same table but with unchanged wording for the operations?

^{out of scope idea follows:
We could remove the 'list items' one completely by moving the menu-title and menu-description text fields to the 'list' page (above the table that lists all menu items) and rename that page to 'edit'}

Ignore that for now, screenshot of same without changing the labels would be the next step please.

subscribe.

I like the new layout very much.

Some comments:

Menu title is listed as "Main menu (Menu name: main-menu)", would it be better to replace "Menu name:" with "Machine name:". See the content types (/admin/build/types) which uses "Machine name:".

There may be some white space issues, too. Did you try coder on this patch?

When creating a new menu, the Menu Name field description is:

"The machine-readable name of this menu. This text will be used for constructing the URL of the menu overview page for this menu. This name must contain only lowercase letters, numbers, and hyphens, and must be unique."

So, to be consistent I'd use "machine name".

I do not see any other issue.

Forgot the change the status. ;-)

OK. Now from my point of view it's RTBC.

Thanks.

Doesn't work correctly for me. See screenshot.

Hmm, I refreshed the page a few times and it just worked - theme registry or something?

Re-rolled with lower case operations, no other changes.

Hey Dries! Each time I applied this patch I had to clear the menu cache.

Steps:
1. apply the patch
2. visit the module page (which resets the menu cache)
3. visit the menu (new) page
4. remove the patch
5. visit the module page
6. visit the menu (old before patch) page

Cheers.

Reviewed the last patch against HEAD. RTBC.

Please delete the menu cache or visit the module page once the patch is applied. ;-)

What's the purpose of

 function block_add_block_form_validate($form, &$form_state) {
+  $test = 'test';

Leftover debug statement? (I couldn't find another reference to it in the patch). Back to CNW but if I'm wrong feel free to return this to RTBC.

Machine name isn't run through t().

Reviewed the last patch against HEAD. The test code in #15 properly removed. Ready to be comited.

This patch even work with the new user and admin menu splits comited earlier.

Please delete the menu cache or visit the module page once the patch is applied. ;-)

Sorry.

Lets *not* add this machine name thing, it adds no real value and should be left out of the interface (atleast on this one). The patch is starting to look good, can I have some screenshots (or is the last one up to date?)

This patch goal is to have consistency between screens. The admin menu layout should be the same as the admin content types page. And on Content types, we are using "Article (Machine name: article)". So for consistency, we are adding the same information in this patch for the admin menu page.

Removing "Machine name:" is another issue. And if we are to remove "Machine name:", we will need to remove it for Content type as well.

So I guess we should commit this patch, with the t() correction. And later, in another issue, remove "Machine name:" everywhere it's used.

Content types admin doesn't put 'machine name' through t(), either, not sure if that's on purpose or not.

However admin/build/types is the exception rather than the rule - admin/user/roles and many other pages don't have machine name at all, so I'm not sure it's necessary to include here in the name of consistency, and apart from that niggle I think this is RTBC.

I have to agree with catch, admin/build/types is an exception rather then the set standard for this. I think this is RTBC, after removing that small thing.

Also theme_menu_admin_overview() is missing phpdoc

Extra blank line here:

-  return theme('admin_block_content', $content);
+

although I prefer a line break before returns though, not sure what the official line is.

ack, sorry for multiple bumps:

'foo'.'bar'

this is D6 style. you want this in stead:

'foo' . 'bar'

Looks much better. I just realised there's a while (db_fetch_array()) but it's not actually touched by the patch so silly to hold up because of that.

Leaving at CNR for the look, but that also seems really good to me.

Looking good to me, the labels are a bit awkward but that's garland-ish issue.

Good to go. Do it!

Committed to CVS HEAD. Thanks!

'item' needs to be changed to 'link'

changes the text in the table, plus other uses in the same file.

This looks good to me.

two minor changes required to make the tests look for the correct strings in the UI.

Now that the testbot is happy, let me try this again.

We should stick to menu “item”, not menu “link”.

A menu item is composed of:

• title
• link (path)
• description
• weight

@38: this patch is only making sure we use the terms that were already in use in HEAD. If you want to change the wording, you should open another issue. Also, not fixed yet because the patch has to be committed first.

As yoroy mentions, the existing help text already mostly uses "link".

At admin/build/menu:

Menus are a collection of links used to navigate a website. [...] Select an existing menu from this list to manage its menu links.

At admin/help/menu there is one "items" but mostly "links":

[...] Menus are a hierarchical collection of links used to navigate a website. [...] The Management menu contains links for administration and content creation, while the Navigation menu is the default location for site navigation links created by newly enabled modules. [...] Most Drupal themes also provide support for the Main links and Secondary links, by displaying them in either the header or footer of each page. The Main menu is the default source for the Main links and the User menu is the default source for the Secondary links. By default, the User menu has links to take the current user to their account or allow them to log out, while the Main menu and Secondary menu contain no menu links but may be configured to contain custom menu items specific to your site. [...]

[...] Select a menu from this list to add or edit a menu link, or to rearrange links within the menu.

(emphasis added)

@xmacinfo - We didn't have time to clear up the distinction in D6 when we split menu router items from menu links. In D5, there was no distinction, hence the carry-over of the use of 'item' in D6 in many places where it's not accurate.

Ideally we should think some other distinct term for router items that avoids the word 'item'.

Committed follow-up. Thanks!

I'm reopening this one more time: the lengthy descriptions in the first column push the operations columns and make them span two lines:

Let's add a nowrap to the operations column to avoid that:

Yes please.

Aww my comment dissapeared by a Drupal white page, but after carefull consideration taking yoroys suggestion into account : Yes

We need go take into account translation. Some translated strings take a lot more space. Also, that type of changes should be theme overdidable.

We had a similar nowrap on another admin screen, and it completely broke when translated. This needs to be handled with width or similar.

Then I say we don't handle it at all and work on rewriting the descriptions (not here!) / leave it to the themes to fix it.

Sounds good to me. Marking back to fixed.