The admin pages have a lists of menus to navigate between the various screens. These menu lists have small touch targets because the links are just the single word of the admin item.

We need to re-write the html and the css to make the touch target much bigger. This has a desktop benefit too. We can do that by making the entire menu item and its description as part of the link.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

First go at a patch. I've left the visual styling very similar to how it was before.

We need to discuss feedback from touch and hover devices but I'd rather get a code review first I had to write php(!)

LewisNyman’s picture

Status: Active » Needs review
JohnAlbin’s picture

Issue tags: +Novice

Tagging this as novice.

Fr0s7’s picture

Tested, and works nicely, but because of the new markup nesting, the hover states looked ugly on the description text. This patch:

  • Cleans up description text underlines on hover
  • Adds a background color on hover and active states
jtwalters’s picture

I tested out the patch and it works for me in the latest Chrome, Opera, and IE 8.

JohnAlbin’s picture

Issue tags: -Novice +mobile-novice

Using proper mobile-novice tag

JohnAlbin’s picture

Title: Make admin list touch friendly. » Increase touch size of admin menu lists
JohnAlbin’s picture

Title: Increase touch size of admin menu lists » Increase touch target size of admin menu lists
Status: Needs review » Needs work

There's no reason we have to change the wrapper of the description from a div to a span. In HTML5, <a> tags can wrap block elements and still validate.

+ul.admin-list li a:hover .label,
+ul.admin-list.compact li a:hover .label {

We need an a:active selector there as well (as the earlier rulesets already include). :hover is only for desktop.

jtwalters’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
3.22 KB

First try here...so let me know if I'm doing this wrong.

Attached is the new patch and the interdiff.

Status: Needs review » Needs work

The last submitted patch, 1488962-4-interdiff.patch, failed testing.

jtwalters’s picture

Status: Needs work » Needs review

Oops, should have named that interdiff.txt.

JohnAlbin’s picture

FileSize
4.19 KB

I'm not crazy about the yellow styling on hover; it's not a part of Seven's current color palette. I tried white for hover styling it worked much better.

I looked at the CSS for this and realized that the CSS before the patch was more complicated then it needed to be. There were lots of unused selectors and redundant properties.

So I took the patch from #9 (Thanks, Joel!) and simplified the entire set of CSS. That's why there's so many minuses in the patch.

jtwalters’s picture

I like the simplicity and harmony of the white color background as well.

I tried out the patch in #12 and it is working fine. Thanks, John Albin!

JohnAlbin’s picture

@jtwalters Maybe mark it as RTBC? :-D

droplet’s picture

+++ b/core/themes/seven/style.cssundefined
@@ -377,50 +376,41 @@ ul.inline li {
+  padding: 2px 19px;

This is not left aligned, looks a bit strange to me. If this is increase the click area. I suggest add more top&bottom padding. and it also adding extra space to all admin pages.

Other admin pages have not hover effect. It broken theme style's consistency.

LewisNyman’s picture

It should be noted that most other admin listing pages do not use the mark up for admin lists but rather use tables. Converting them over to the same mark up is being handled in #1276908: Administrative tables are too wide for smaller screens. Once all listing pages are using the same mark up we should achieve consistency.

Bojhan’s picture

Issue tags: +Usability

Can I get a screen? Please mark it for Usability whenever it changes anything visually.

jtwalters’s picture

Here are some screenshots with the patch from #12

droplet’s picture

@jtwalters,

what's your system ?? your fonts are bigger than mine a lot.

jtwalters’s picture

@droplet this was from a retina macbook pro so I guess the screenshots were dimension doubled. e.g. 200% zoom

mcjim’s picture

Status: Needs review » Reviewed & tested by the community

#12 looks good to go.
Have tested in IE8/9, FF, Chrome, iOS Simulator.

catch’s picture

Status: Reviewed & tested by the community » Fixed

I tested this quickly and it seems fine to me. I'm not 100% about underlining the links on hover as well as the white background, but no-one else brought it up so it might just be me, and it's very minor compared to the rest of the changes in the patch. Committed/pushed to 8.x.

catch’s picture

Status: Fixed » Needs work

Actually no, I wrote that comment before I committed/pushed the patch, and it no longer applies.

mcjim’s picture

Status: Needs work » Needs review
FileSize
3.68 KB

Rerolled patch.

jtwalters’s picture

#24 applied cleanly to latest 8.x and looks good to go

mcjim’s picture

Status: Needs review » Reviewed & tested by the community
Bojhan’s picture

Status: Reviewed & tested by the community » Needs review

Wait, wtf, why are we adding this styling?

mcjim’s picture

FileSize
3.66 KB

After discussion with @bojhan, removed background change on hover.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community
Ken Hawkins’s picture

Just a quick chime in that it applied well here too and looks like a general usability win.

jtwalters’s picture

confirmed #28 applies. looks good to me. i don't really care either way about the background color, since the underline is enough of a hover indicator.

edward_or’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
12.5 KB

Patch looks great so far. It does not, however, apply as you would expect to the node/add page (the node add page has its own theme function theme_node_add_list) as the a tag in the node add list does not wrap the description.

This patch changes the markup from seven_node_add_list to match the output of seven_admin_block_content this patch creates.

mcjim’s picture

Status: Needs review » Needs work

Well noticed :-)

It does perhaps raise the question of whether we should have theme_node_add_list() and not use theme_admin_block_content()…

Patch changes vertical tabs on the node/add form to collapsible fieldsets. Was this cross-pollination from another patch?

mcjim’s picture

Status: Needs work » Needs review
FileSize
4.34 KB

Re-rolled to include changes to seven_node_add_list() from #32.

edward_or’s picture

Patch changes vertical tabs on the node/add form to collapsible fieldsets. Was this cross-pollination from another patch?

Well spotted :)

Thanks for fixing that.

h4rrydog’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

- Confirmed #34 applies.
- Touch targets have been enlarged as previous.
- Vertical tabs on node/add form remain vertical tabs.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/seven/template.phpundefined
@@ -70,11 +72,14 @@ function seven_admin_block_content($variables) {
-      $output .= '<li class="leaf">';
-      $output .= l($item['title'], $item['href'], $item['localized_options']);
+      $output .= '<li>';
+      $content = '<span class="label">'.$item['title'].'</span>';

Missing spaces around $item['title'].

Also, if 'html' is TRUE, text is no longer sanitized. I think his patch may be introducing a security vulnerability.

mcjim’s picture

Status: Needs work » Needs review
FileSize
4.36 KB

Patch attached corrects missing spaces around $item['title']

$content, which is the first argument for l(), uses $item['title'] and $item['description']. These are now both passed through filter_xss_admin().

Status: Needs review » Needs work

The last submitted patch, 1488962-38-admin-list.patch, failed testing.

moshe weitzman’s picture

These lists will be deemphasized by the mobile toolbar but this is still worth doing.

mcjim’s picture

Status: Needs work » Needs review

#38: 1488962-38-admin-list.patch queued for re-testing.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

In the testing we did so far, these listings are by no means deemphasized. People end up on them all the time, especially in configuration where we have no depth beyond the items.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/themes/seven/template.phpundefined
@@ -47,8 +47,10 @@ function seven_node_add_list($variables) {
+      $content = '<span class="label">' . $type->name . '</span>';

Hm. While we're at it, does this need to be check_plain()ed?

chellman’s picture

check_plain sounds like a good idea to me. Patch re-rolled.

Status: Needs review » Needs work

The last submitted patch, drupal-adminlist-1488962-44.patch, failed testing.

chellman’s picture

Status: Needs work » Needs review

#1798732 was reverted, so the test actually succeeded.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks sensible. Committed/pushed to 8.x.

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

Anonymous’s picture

Issue summary: View changes

Improving issue description