More Seven Theme issues: #1986434: New visual style for Seven

Problem/Motivation

We aim to tweaks the styling of the navigation listings to bring them more inline with the new Seven visual language.

We developed Proposal: A Style Guide for Seven. This issue aims to introduce the proposed styling for pagination in core.

To quote the rationale provided:

The Nav List is used on “hub” pages to provide an on-page navigation menu with help text. This component is not greatly changed from D7, with only minor typographic finessing and a rounding of the “fancy bullet” used to mark the list items, in keeping with the friendliness of the brand.

Proposed resolution

In #1945552: Add Nav List component we laid the ground work for this change.

18.nav-list.png

Remaining tasks

  • Make a patch

User interface changes

This is a style only change,

API changes

No API Changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

Status: Active » Needs review
FileSize
3.68 KB

Here's the initial patch

Status: Needs review » Needs work
Issue tags: -Usability, -styleguide

The last submitted patch, new-admin-list-style.patch, failed testing.

yoroy’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +styleguide

#1: new-admin-list-style.patch queued for re-testing.

echoz’s picture

Status: Needs review » Needs work

Lookin' good.
* The .png is misnamed in the rtl file ("list-icon-rtl.png" rather than "nav-list-icon-rtl.png")
* git apply reports trailing whitespace on the pngs.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
3.98 KB

Thanks, I changed the class in the RTL file. Not sure about the whitespace in the png. I ran them through imageoptim....

echoz’s picture

FileSize
3.69 KB

#5 patch changed the wrong unrelated img ruleset. Here is that correction. Patch has the same git warning, but applies and stages fine. Implementation looks fine.
-------

2016875-nav-list-6.patch:26: trailing whitespace.
?PNG
2016875-nav-list-6.patch:35: trailing whitespace.
?PNG
warning: 2 lines add whitespace errors.

Bojhan’s picture

Can we fix these whitespace errors?

The only thing I am concerned about is the fact that it vertically centers to the title. That looks a bit strange, but it looks totally fine without descriptions. Given that many content facing occasions there won't be descriptions. I am fine with leaving that, also with typographic changes it will look better.

echoz’s picture

@Bojhan, not sure what you mean about "vertically centers to the title". With desciptions, I would think this is desired, same as previously.

The images were not transparent, they were on white. Since they are on a non white background on the config page, I've re-done the images from seventy-eight's PSD (plus optimized). I was hoping the whitespace warnings would go away, but they are the same. Note they are warnings not errors.

Only change in this patch is transparent images.

nav-list-config.png

nav-list-reports.png

LewisNyman’s picture

The icon looks good aligned with the title, @Bojhan if you want to we could add a variant class if a description exists?

yoroy’s picture

Middle alignment of icon and title, with or without a description is the way to go. I don't think a variant class is needed.

echoz’s picture

Adjustment for previously untested RTL

nav-list-config-rtl.png

Bojhan’s picture

@echoz Nice work!

echoz’s picture

rteijeiro’s picture

rteijeiro’s picture

FileSize
9.74 KB

The patch :(

LewisNyman’s picture

The last patch seems a little bit overreaching, should we make the changes here or as part of #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute ?

rteijeiro’s picture

FileSize
9.74 KB

You are right @Lewis, I have included some crap from other patch, sorry :(

This is the right patch ;)

echoz’s picture

FileSize
3.62 KB

Hold on. First of all, the patches in #15 or #17 (which are identical, btw) leave out the new icons. They are changing the rtl attribute selector in seven's style.css to have quotes. But #2030925: quote rtl attribute selector has not landed yet. Whichever patch gets committed first (2030925 or this one), the other will need to be re-rolled. The patch that will work right now is #13, which I am resubmitting here. If #2030925: quote rtl attribute selector gets committed before it, I will re-roll this with changing I think only one needed "rtl".

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

I agree with @echoz. Sorry for the noise :(

Patch applies well and seems to work well.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

echoz’s picture

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

Status: Needs review » Needs work
Issue tags: -Usability, -styleguide, -RTBC July 1

The last submitted patch, 2016875-nav-list-21.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +styleguide, +RTBC July 1

#21: 2016875-nav-list-21.patch queued for re-testing.

YesCT’s picture

rtl is different than in #11.
is that because the fix for rtl will be in another issue?

admin/config

no patch

s01-nopatcch-admin_config.png

no patch - rtl

s01-nopatch-rtl-admin_config.png

patch

s01-patch-admin_config.png

patch - rtl

s01-patch-rtl-admin_config.png

admin/reports

no patch

s02-nopatch-admin_reports.png

patch

s02-patch-admin_reports.png

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +CSS, +CSS novice

Yep, we could of easily lost some RTL styling in the past week.

YesCT’s picture

Issue tags: +frontend, +D8MI

tagging for d8mi frontend people.

echoz’s picture

Status: Needs work » Needs review
FileSize
3.72 KB

It was a specificity issue that my browser cache fooled me into thinking I had fixed.

echoz’s picture

The column css was screwed up (previous to this issue) and issue only showed up on RTL. Fixed.

patched LTR

admin-config-ltr.png

patched RTL

admin-config-rtl.png

Status: Needs review » Needs work
Issue tags: -CSS, -Usability, -styleguide, -frontend, -D8MI, -CSS novice, -RTBC July 1

The last submitted patch, 2016875-nav-list-28.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
Issue tags: +CSS, +Usability, +styleguide, +frontend, +D8MI, +CSS novice, +RTBC July 1

#28: 2016875-nav-list-28.patch queued for re-testing.

echoz’s picture

FileSize
4.66 KB

Removed indentatation on "Hide description" display, as per #2037525: 'Hide description' layout bug

echoz’s picture

FileSize
18.02 KB

…and a screenshot

no-desc-no-indent.png

Status: Needs review » Needs work
Issue tags: -CSS, -Usability, -styleguide, -frontend, -D8MI, -CSS novice, -RTBC July 1

The last submitted patch, 2016875-nav-list-31.patch, failed testing.

echoz’s picture

Status: Needs work » Needs review
Issue tags: +CSS, +Usability, +styleguide, +frontend, +D8MI, +CSS novice, +RTBC July 1

#31: 2016875-nav-list-31.patch queued for re-testing.

echoz’s picture

This still applies and should be easy to review (admin/config and admin/reports), can we move this along?

YesCT’s picture

I swear I think someone at NYCCamp made all the before and after screenshots and double checked this...

Bojhan’s picture

This smells like RTBC

echoz’s picture

I remember somewhere it was said we should leverage the icons from ry5n's Libricons #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core for the icons used here. In #1963886: Use HiDPI icons in the toolbar only the icons used in toolbar were added in misc/icons, which included chevron-disc up + down but not left + right, which are part of Libricons and the ones used here in the admin nav lists.

This new patch is the same as #31 patch, which still applys and was ready for rtbc, plus uses the svg (w/ Modernizer png fallback) from misc/icons.

It adds these icons, in the right color, at misc/icons:

chevron-disc-left.png
chevron-disc-left.svg
chevron-disc-right.png
chevron-disc-right.svg

It does not add the pngs (created from seventy-eight's PSDs) that had been used up to the #31 patch. If we decide not to use the svg / png from misc/icons, we can use the #31 patch.

Using svg icons from misc/icons

nav-list-svg-icon.png

echoz’s picture

tagging

rteijeiro’s picture

Patch applies well (except for the errors regarding existing icon files) and looks really good.

Maybe it's a RTBC?

echoz’s picture

FileSize
13.84 KB

The git warnings (not errors) have always been there for the pngs.

I wanted to hear if nav-list is a candidate for switching to use svg icons, which I assume is true and what this last patch #38 does. Here's a screenshot showing that ie8 gets the Modernizer png fallback.

ie8-nosvg.png

RTL should be tested since this change (admin/config and admin/reports).

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
93.78 KB
90.81 KB
257.52 KB
289.01 KB

It looks ok for me. Tested for RTL and it seems to fix a bug in admin/config page. Take a look at the screenshots:

admin/config BEFORE:

rtl-admin-config-before.png

admin/config AFTER:

rtl-admin-config-after.jpeg

admin/reports BEFORE:

rtl-admin-reports-before.png

admin/reports AFTER:

rtl-admin-reports-after.png

Well done echoz!!

Maybe RTBC?

echoz’s picture

Thanks for the RTL testing!
Correct, I discovered and fixed that column bug in #28 that existed previous to this issue but only showed up on RTL.

kfritsche’s picture

Just tested it again with LTR and RTL.
Still works - RTCB
No reroll needed.

yoroy’s picture

Yes, this is ready to get in!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

That looks much better!

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

added related Use Libricons meta issue, removed 1849712