More Seven Theme issues: #1986434: 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

Issue tags: +Usability, +styleguide

taggin

LewisNyman’s picture

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

tags

LewisNyman’s picture

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.png

Screen Shot 2013-06-27 at 10.30.04.png

LewisNyman’s picture

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

enginpost’s picture

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.

enginpost’s picture

Status: Needs work » Needs review
FileSize
6.77 KB

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

rteijeiro’s picture

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.

tkoleary’s picture

@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.

LewisNyman’s picture

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.

LewisNyman’s picture

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.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
14.91 KB

Attempt at a re-roll.

Bojhan’s picture

This smells RTBC.

tkoleary’s picture

Looks great.

Bojhan’s picture

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

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.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
17.46 KB
4.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.

Bojhan’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

Bojhan’s picture

This kind of messes up tabs, is that intentional?

LewisNyman’s picture

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.

yoroy’s picture

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

Bojhan’s picture

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.

LewisNyman’s picture

Status: Postponed » Needs work
FileSize
142.97 KB

I noticed we are missing the hover icon on rtl.

Screen Shot 2013-10-08 at 14.26.12.jpg

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
17.5 KB
741 bytes
Kendall Totten’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Postponed

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

LewisNyman’s picture

Status: Postponed » Needs work

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

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

LewisNyman’s picture

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

LewisNyman’s picture

Status: Needs work » Needs review
idebr’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
28.13 KB
6.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
LewisNyman’s picture

Status: Needs work » Needs review
Parent issue: » #1986434: New visual style for Seven
FileSize
489 bytes
17.79 KB

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

Bojhan’s picture

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

Status: Reviewed & tested by the community » Needs work
FileSize
16.49 KB
10.89 KB
11.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. :(

webchick’s picture

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.

Bojhan’s picture

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 :)

LewisNyman’s picture

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.

webchick’s picture

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. :)

LewisNyman’s picture

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.

manooh’s picture

Assigned: Unassigned » manooh

I'm having a look.

Bojhan’s picture

@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.

webchick’s picture

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.

webchick’s picture

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

Bojhan’s picture

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.

LewisNyman’s picture

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.

LewisNyman’s picture

Status: Needs work » Needs review
LewisNyman’s picture

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