More Seven Theme issues: #1986434: [meta] New visual style for Seven

Problem/Motivation

We've designed an update for the content header in Seven. The color and spacing changes bring it inline with the rest of the new style guide. The new "Add to shortcuts" icon has a stronger association with the task. More controversially, the breadcrumbs have been removed.

We developed Proposal: A Style Guide for Seven.

To quote the rationale provided:

In D7, the “add to shortcuts” icon is a circled plus symbol. This symbol is commonly associated with an add or create action, but there is nothing to indicate the connection to shortcuts. We propose using the star icon globally for shortcuts in Seven to make it more recognizable and understandable to a wide audience. Most people will be familiar with the use of the star icon for bookmarks/favourites from their web browser (IE, Chrome, Firefox) and from popular services such as Twitter.

Proposed resolution

In #1938418: Add Content Header component we laid the ground work for this change.

new-content-header-screen.png

Remaining tasks

  • Make a patch

User interface changes

This proposal removes the breadcrumbs from the admin interface. With the new toolbar, which stays open between page loads, it's possible for users to traverse back up the navigation tree.

API changes

No API Changes

Files: 
CommentFileSizeAuthor
#47 content-header-2022695-47.patch18.67 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 59,239 pass(es).
[ View ]
#47 interdiff.txt894 bytesLewisNyman
#33 shortcuts-overlay.png6.31 KBidebr
#33 shortcuts-alignment.png28.13 KBidebr
#27 interdiff.txt741 bytesLewisNyman
#27 content-header-2022695-26.patch17.5 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 58,848 pass(es).
[ View ]
#26 Screen Shot 2013-10-08 at 14.26.12.jpg142.97 KBLewisNyman
#19 interdiff.txt4.86 KBLewisNyman
#19 content-header-2022695-19.patch17.46 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 58,713 pass(es).
[ View ]
#14 content-header-2022695-13.patch14.91 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 58,276 pass(es).
[ View ]
#7 content-header-2022695-7.patch6.77 KBenginpost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-header-2022695-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 Screen Shot 2013-06-27 at 11.19.57.png85.47 KBLewisNyman
#3 content-header-2022695.patch14.32 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-header-2022695.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 Screen Shot 2013-06-27 at 10.29.51.png50.67 KBLewisNyman
#3 Screen Shot 2013-06-27 at 10.30.04.png80.69 KBLewisNyman
new-content-header-screen.png10.1 KBLewisNyman

Comments

Issue tags:+Usability, +styleguide

taggin

Issue tags:+CSS, +Novice, +css-novice

tags

Status:Active» Needs review
Issue tags:+styleguide-navigation
StatusFileSize
new80.69 KB
new50.67 KB
new14.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-header-2022695.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, here's the look and feel implementation for the content header. It's clear that this implementation breaks the current tab styling. This issue is reliant on #1490402: Redesign tabs and the content header. I'm working on getting them together in a sandbox.

Screen Shot 2013-06-27 at 10.29.51.pngScreen Shot 2013-06-27 at 10.30.04.png

StatusFileSize
new85.47 KB

Here's a picture of the content header patch working with the latest patch from #1490402: Redesign tabs and the content header.

Screen Shot 2013-06-27 at 11.19.57.png

Status:Needs review» Needs work
Issue tags:+CSS, +Usability, +Novice, +styleguide, +css-novice, +styleguide-navigation

The last submitted patch, content-header-2022695.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch content-header-2022695-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled the patch. I hope I was able to incorporate all of the right modifications.

Status:Needs review» Needs work

This patch removes the core/modules/shortcut/images/shortcut-add.png image but doesn't add the referenced core/modules/shortcut/images/favstar.svg image.

@enginpost

Looks like this patch is failing now since it's probably out of sync with changes to head. Could you re-roll it? I'd really like to test it out.

Status:Needs work» Needs review
Issue tags:-CSS, -Usability, -Novice, -styleguide, -css-novice, -styleguide-navigation

#7: content-header-2022695-7.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, content-header-2022695-7.patch, failed testing.

Status:Needs work» Needs review

#7: content-header-2022695-7.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+CSS, +Usability, +Novice, +styleguide, +css-novice, +styleguide-navigation

The last submitted patch, content-header-2022695-7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,276 pass(es).
[ View ]

Attempt at a re-roll.

This smells RTBC.

Looks great.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:-Novice, -css-novice+styleguide-add-icons

Sorry! Now that #1963886: Use HiDPI icons in the toolbar has paved the way we can add the icons using the same technique.

Status:Needs work» Needs review
StatusFileSize
new17.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,713 pass(es).
[ View ]
new4.86 KB

I've re-rolled and re-implemented the icon in line with the implementation in #1963886: Use HiDPI icons in the toolbar

Status:Needs review» Needs work

The last submitted patch, content-header-2022695-19.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

Back to RTBC

This kind of messes up tabs, is that intentional?

I think so, the tabs patch has to accommodate the changes to the content header, so it should look correct when the two patches are applied together.

Reviewing styleguide issues with webchick: Just another cross-reference to #1490402: Redesign tabs and the content header then.

Status:Reviewed & tested by the community» Postponed

Postponing this after discussion with webchick, we can put it back to RTBC when Redesign tabs is RTBC.

Status:Postponed» Needs work
StatusFileSize
new142.97 KB

I noticed we are missing the hover icon on rtl.

Screen Shot 2013-10-08 at 14.26.12.jpg

Status:Needs work» Needs review
StatusFileSize
new17.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,848 pass(es).
[ View ]
new741 bytes

Status:Needs review» Reviewed & tested by the community

I applied the patch, tested by clicking the favorite icon on an admin page, and then unfavoriting admin pages. Checked the hover state as well.
RTBC on RTL and LTR in Chrome & Firefox.

Tested as part of the #dcATL code sprint.

Status:Reviewed & tested by the community» Postponed

Setting back to be postponed on #1490402: Redesign tabs and the content header

Status:Postponed» Needs work

Let's add the breadcrumbs back below the secondary tabs for now.

See https://drupal.org/node/1953374#comment-8014161

StatusFileSize
new17.79 KB
PASSED: [[SimpleTest]]: [MySQL] 59,838 pass(es).
[ View ]

I'd added back the breadcrumb but under the secondary tabs

Status:Needs work» Needs review

Issue summary:View changes
Status:Needs review» Needs work
StatusFileSize
new28.13 KB
new6.31 KB
  1. Icon alignment is off in Windows: Firefox and IE10, see shortcuts-alignment.png
  2. New 'star'-icon is almost invisible with overlay enable, see shortcuts-overlay.png

Status:Needs work» Needs review
Parent issue:» #1986434: [meta] New visual style for Seven
StatusFileSize
new489 bytes
new17.79 KB
PASSED: [[SimpleTest]]: [MySQL] 60,414 pass(es).
[ View ]

Thanks, I've applied the vertical alignment using pixels, I think it's a lot more reliable than the keywords.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new16.49 KB
new10.89 KB
new11.22 KB

I'm going off the issue summary, which says this has to do with the shortcut icon.

However, in all of my browsers, as well as simplytest.me (to hopefully rule out any weird caching issues), this is what I see:

When looking at a page like e.g. Structure, the shortcut link is completely imperceptible against the background of Overlay:

Star outline is imperceptible against Overlay

On a page like Content (which is already in Shortcuts) it does work, but the alignment seems very off:

Star near the bottom of the kearning

And then in hover state, the +/- symbol once again seems like it's in a really weird place, and again really hard to see against Overlay:

Grey of add / remove gets lost in dark grey of overlay, and is positioned up and to the right of the star

It's possible this is because we need another patch in first, in which case we should postpone this on that, but atm this doesn't look committable, sorry. :(

Also, unless I missed it, we seem to have lost the hover text to explain what the star is for. I don't think it's intuitive enough to go without that. It's fine if it's a title attribute or whatever, rather than custom styling.

So we do need the other patch to go in, darn I probably should have put it on postponed.

However this review is really nice, there are things that we should address. We should probably keep the text upon hover.

@webchick Actually the biggest thing this patch does is make the spacing and typography nicer :)

Yes this does break overlay I don't think it's specific enough that this patch only covers the non-overlay page. Webchick also raises a good point about the semantics of the icon.

Part of the justification of the star icon is that it has already gained a semantic meaning from browsers and other web services for many years. With this in mind I think there is as much semantic meaning as there was before. Both the old and the current solution still don't inform the user of where these shortcuts sit, but that is another issue.

I've seen the star before, but in the context of "liking" something. (At least on Twitter.) In Google Code's issue tracker, it's a synonym for "Follow this issue." I've never really seen it used in the context of "shortcuts," although now that I say this I laugh as I see in my address bar in Firefox there's a star on the far right there that I've never noticed before that is indeed blue on a page that I have bookmarked. Sheesh.

Anyway, given that the star symbol seems ambiguous in at least 3 ways, specifying which way we mean it can't hurt. :)

It could be ambiguous, but we tend to go overboard in Drupal with helper text just incase, which adds a lot of noise. I guess this would only show up on hover though? How about something as simple as a title attribute? The old styling was kind of fragile on narrower screens.

Assigned:Unassigned» manooh

I'm having a look.

@Lewis I am not sure if its that weird. Most browsers show a "Selection" dialog when pressing it, we do nothing. I don't necessarily like relying on a title element to communicate purpose. But obviously would be upon hover.

As I said in #37, I think a title attribute would be fine, at least for the purposes of this issue. We could always explore other options later.

And FWIW, a "title attribute"-like behaviour is what Firefox, Chrome, etc. does when I hover over its star.

Can anyone update the patch using the title attribute? I dont really know how to solve this elegantly otherwise, because on mobile adding loads of text is pretty much unpossible.

Status:Needs work» Needs review
Issue tags:+needs accessibility review
Related issues:+#1490402: Redesign tabs and the content header
StatusFileSize
new894 bytes
new18.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,239 pass(es).
[ View ]

Ok, I've added a title attribute. I don't think this will causes an accessibility regression but it's worth checking.

Status:Needs review» Needs work

The last submitted patch, 47: content-header-2022695-47.patch, failed testing.

Status:Needs work» Needs review

47: content-header-2022695-47.patch queued for re-testing.

Status:Needs review» Closed (duplicate)
Issue tags:-styleguide-navigation