Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#44 | drupal-adminlist-1488962-44.patch | 4.38 KB | chellman |
#38 | 1488962-38-admin-list.patch | 4.36 KB | mcjim |
#34 | 1488962-34-admin-list.patch | 4.34 KB | mcjim |
#32 | 1488962-32-admin-list.patch | 12.5 KB | edward_or |
#28 | 1488962-28-admin-list.patch | 3.66 KB | mcjim |
Comments
Comment #1
LewisNyman CreditAttribution: LewisNyman commentedFirst 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(!)
Comment #2
LewisNyman CreditAttribution: LewisNyman commentedComment #3
JohnAlbinTagging this as novice.
Comment #4
Fr0s7 CreditAttribution: Fr0s7 commentedTested, and works nicely, but because of the new markup nesting, the hover states looked ugly on the description text. This patch:
Comment #5
jtwalters CreditAttribution: jtwalters commentedI tested out the patch and it works for me in the latest Chrome, Opera, and IE 8.
Comment #6
JohnAlbinUsing proper mobile-novice tag
Comment #7
JohnAlbinComment #8
JohnAlbinThere'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.
We need an
a:active
selector there as well (as the earlier rulesets already include). :hover is only for desktop.Comment #9
jtwalters CreditAttribution: jtwalters commentedFirst try here...so let me know if I'm doing this wrong.
Attached is the new patch and the interdiff.
Comment #11
jtwalters CreditAttribution: jtwalters commentedOops, should have named that interdiff.txt.
Comment #12
JohnAlbinI'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.
Comment #13
jtwalters CreditAttribution: jtwalters commentedI 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!
Comment #14
JohnAlbin@jtwalters Maybe mark it as RTBC? :-D
Comment #15
droplet CreditAttribution: droplet commentedThis 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.
Comment #16
LewisNyman CreditAttribution: LewisNyman commentedIt 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.
Comment #17
Bojhan CreditAttribution: Bojhan commentedCan I get a screen? Please mark it for Usability whenever it changes anything visually.
Comment #18
jtwalters CreditAttribution: jtwalters commentedHere are some screenshots with the patch from #12
Comment #19
droplet CreditAttribution: droplet commented@jtwalters,
what's your system ?? your fonts are bigger than mine a lot.
Comment #20
jtwalters CreditAttribution: jtwalters commented@droplet this was from a retina macbook pro so I guess the screenshots were dimension doubled. e.g. 200% zoom
Comment #21
mcjim CreditAttribution: mcjim commented#12 looks good to go.
Have tested in IE8/9, FF, Chrome, iOS Simulator.
Comment #22
catchI 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.
Comment #23
catchActually no, I wrote that comment before I committed/pushed the patch, and it no longer applies.
Comment #24
mcjim CreditAttribution: mcjim commentedRerolled patch.
Comment #25
jtwalters CreditAttribution: jtwalters commented#24 applied cleanly to latest 8.x and looks good to go
Comment #26
mcjim CreditAttribution: mcjim commentedComment #27
Bojhan CreditAttribution: Bojhan commentedWait, wtf, why are we adding this styling?
Comment #28
mcjim CreditAttribution: mcjim commentedAfter discussion with @bojhan, removed background change on hover.
Comment #29
Bojhan CreditAttribution: Bojhan commentedComment #30
Ken Hawkins CreditAttribution: Ken Hawkins commentedJust a quick chime in that it applied well here too and looks like a general usability win.
Comment #31
jtwalters CreditAttribution: jtwalters commentedconfirmed #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.
Comment #32
edward_or CreditAttribution: edward_or commentedPatch 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.
Comment #33
mcjim CreditAttribution: mcjim commentedWell 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?
Comment #34
mcjim CreditAttribution: mcjim commentedRe-rolled to include changes to seven_node_add_list() from #32.
Comment #35
edward_or CreditAttribution: edward_or commentedWell spotted :)
Thanks for fixing that.
Comment #36
h4rrydog CreditAttribution: h4rrydog commentedLooks good to me.
- Confirmed #34 applies.
- Touch targets have been enlarged as previous.
- Vertical tabs on node/add form remain vertical tabs.
Comment #37
Dries CreditAttribution: Dries commentedMissing spaces around $item['title'].
Also, if 'html' is TRUE, text is no longer sanitized. I think his patch may be introducing a security vulnerability.
Comment #38
mcjim CreditAttribution: mcjim commentedPatch 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()
.Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedThese lists will be deemphasized by the mobile toolbar but this is still worth doing.
Comment #41
mcjim CreditAttribution: mcjim commented#38: 1488962-38-admin-list.patch queued for re-testing.
Comment #42
Bojhan CreditAttribution: Bojhan commentedIn 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.
Comment #43
webchickHm. While we're at it, does this need to be check_plain()ed?
Comment #44
chellman CreditAttribution: chellman commentedcheck_plain sounds like a good idea to me. Patch re-rolled.
Comment #46
chellman CreditAttribution: chellman commented#1798732 was reverted, so the test actually succeeded.
Comment #47
Bojhan CreditAttribution: Bojhan commentedComment #48
catchLooks sensible. Committed/pushed to 8.x.
Comment #49.0
(not verified) CreditAttribution: commentedImproving issue description