Posted by LewisNyman on March 18, 2012 at 11:28pm
27 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | Seven theme |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | d8mux, d8ux, mobile, mobile-novice, Usability |
Issue Summary
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.
Comments
#1
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(!)
#2
#3
Tagging this as novice.
#4
Tested, and works nicely, but because of the new markup nesting, the hover states looked ugly on the description text. This patch:
#5
I tested out the patch and it works for me in the latest Chrome, Opera, and IE 8.
#6
Using proper mobile-novice tag
#7
#8
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:activeselector there as well (as the earlier rulesets already include). :hover is only for desktop.#9
First try here...so let me know if I'm doing this wrong.
Attached is the new patch and the interdiff.
#10
The last submitted patch, 1488962-4-interdiff.patch, failed testing.
#11
Oops, should have named that interdiff.txt.
#12
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.
#13
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!
#14
@jtwalters Maybe mark it as RTBC? :-D
#15
+++ 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.
#16
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.
#17
Can I get a screen? Please mark it for Usability whenever it changes anything visually.
#18
Here are some screenshots with the patch from #12
#19
@jtwalters,
what's your system ?? your fonts are bigger than mine a lot.
#20
@droplet this was from a retina macbook pro so I guess the screenshots were dimension doubled. e.g. 200% zoom
#21
#12 looks good to go.
Have tested in IE8/9, FF, Chrome, iOS Simulator.
#22
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.
#23
Actually no, I wrote that comment before I committed/pushed the patch, and it no longer applies.
#24
Rerolled patch.
#25
#24 applied cleanly to latest 8.x and looks good to go
#26
#27
Wait, wtf, why are we adding this styling?
#28
After discussion with @bojhan, removed background change on hover.
#29
#30
Just a quick chime in that it applied well here too and looks like a general usability win.
#31
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.
#32
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.
#33
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?
#34
Re-rolled to include changes to seven_node_add_list() from #32.
#35
Well spotted :)
Thanks for fixing that.
#36
Looks good to me.
- Confirmed #34 applies.
- Touch targets have been enlarged as previous.
- Vertical tabs on node/add form remain vertical tabs.
#37
+++ 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.
#38
Patch attached corrects missing spaces around
$item['title']$content, which is the first argument forl(), uses$item['title']and$item['description']. These are now both passed throughfilter_xss_admin().#39
The last submitted patch, 1488962-38-admin-list.patch, failed testing.
#40
These lists will be deemphasized by the mobile toolbar but this is still worth doing.
#41
#38: 1488962-38-admin-list.patch queued for re-testing.
#42
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.
#43
+++ 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?
#44
check_plain sounds like a good idea to me. Patch re-rolled.
#45
The last submitted patch, drupal-adminlist-1488962-44.patch, failed testing.
#46
#1798732 was reverted, so the test actually succeeded.
#47
#48
Looks sensible. Committed/pushed to 8.x.
#49
Automatically closed -- issue fixed for 2 weeks with no activity.