Posted by chrisshattuck on March 15, 2009 at 12:55am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | UBUserTesting2009, Usability |
Issue Summary
Identified at UB Usability Testing: http://www.drupalusability.org/node/124
"The admin/build/menu page is formatted as a definition list which makes things look like help. These types of lists are almost always formatted to tables."
The attached patch changes the menu overview page to use a similar structure to the content types management page (admin/build/types). In addition, I have added the 'list', 'edit' and 'add' operations to the listing. I could also add a 'delete' operation for non-core menus, but I wasn't sure if that would be a little too many operations columns.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| menu-overview-before.png | 21.81 KB | Ignored: Check issue status. | None | None |
| menu-overview-after.png | 27.58 KB | Ignored: Check issue status. | None | None |
| menu.admin_.inc_.patch | 2.27 KB | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
#2
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.
#3
subscribe.
#4
yoroy - some good points.
My motivation for the shortened names was mostly to get around the operation columns wrapping. In retrospect, that seems like pretty lame reasoning. :P
Attached is a patch with the update operation column names. I like the off-topic idea above and wouldn't mind tackling that in a subsequent patch if this one gets committed.
#5
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?
#6
Thanks for the comments, xmacinfo.
I used 'Menu name' instead of 'Machine name' because as I understand it, 'machine name' implies only alphanumeric characters and underscores, whereas the menu name can have hyphens. What do you think, is it still a machine name since it's used as a unique, internal identifier?
I ran the patch through coder without any errors, but on review I did see several extra spaces around concatenation, which I have removed in the attached patch.
#7
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.
#8
Forgot the change the status. ;-)
#9
Updated patch with "Machine name" instead of "Menu name".
#10
OK. Now from my point of view it's RTBC.
Thanks.
#11
Doesn't work correctly for me. See screenshot.
#12
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.
#13
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.
#14
Reviewed the last patch against HEAD. RTBC.
Please delete the menu cache or visit the module page once the patch is applied. ;-)
#15
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.
#16
Oh man, that's embarrassing. Re-rolled without line of test code.
#17
Machine name isn't run through t().
#18
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. ;-)
#19
Sorry.
#20
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?)
#21
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.
#22
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.
#23
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.
#24
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.
#25
ack, sorry for multiple bumps:
'foo'.'bar'this is D6 style. you want this in stead:
'foo' . 'bar'#26
Patch is updates with D7 style concatenation, the machine name removed, phpdoc comment added, and a line before the return (for catch ;-) ).
#27
And a screenshot for Bojhan
#28
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.
#29
Looking good to me, the labels are a bit awkward but that's garland-ish issue.
#30
Good to go. Do it!
#31
Committed to CVS HEAD. Thanks!
#32
'item' needs to be changed to 'link'
#33
changes the text in the table, plus other uses in the same file.
#34
The last submitted patch failed testing.
#35
This looks good to me.
#36
two minor changes required to make the tests look for the correct strings in the UI.
#37
Now that the testbot is happy, let me try this again.
#38
We should stick to menu “item”, not menu “link”.
A menu item is composed of:
#39
@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.
#40
As yoroy mentions, the existing help text already mostly uses "link".
At admin/build/menu:
At admin/help/menu there is one "items" but mostly "links":
(emphasis added)
#41
@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'.
#42
Committed follow-up. Thanks!
#43
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:
#44
Yes please.
#45
Aww my comment dissapeared by a Drupal white page, but after carefull consideration taking yoroys suggestion into account : Yes
#46
We need go take into account translation. Some translated strings take a lot more space. Also, that type of changes should be theme overdidable.
#47
We had a similar nowrap on another admin screen, and it completely broke when translated. This needs to be handled with width or similar.
#48
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.
#49
Sounds good to me. Marking back to fixed.
#50
The reason I went with single word descriptions for those links initially was to get around having to worry about the wrap, but now I think it should probably be handled at the theme layer, at the least to be consistent with similar lists in core.
#51
Automatically closed -- issue fixed for 2 weeks with no activity.