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

Problem/Motivation

As well as bringing the pagination more inline with new visual language for Seven, we hope to reduce the UI to serve the majority of cases in Drupal Core.

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:

To simplify the UI and focus on the 80% use case, this guide envisions removing the “first” and “last” links from the pagination component and also the text labels for “previous” and “next” links. This change is contingent on positive user testing.

Proposed resolution

In #1945548: Add pagination component we laid the ground work for this change.

17.pagination.png

Remaining tasks

  • Make a patch for CSS only changes
  • Investigate how we can utlise the views mini pager in core
  • Add icons in separate issue
  • Review accessibility

User interface changes

The 'first' and 'last' buttons will be removed. The labels for 'previous' and 'next' will be removed.

Test pages

  • admin/content (use devel generate to create more than 50 nodes)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

FileSize
1.94 KB

Here's a CSS only patch

LewisNyman’s picture

Status: Active » Needs review
LewisNyman’s picture

Issue summary: View changes

Tweaks to proposed changes

LewisNyman’s picture

Related issues:

#1488866: Redesign pagers for administrative lists.
#538788: Implement Seven pagers as designed

They were closed as duplicates, but they may contain some relevant insights.

xmacinfo’s picture

This is a complete regression! If we go that route it should touch only Seven. Not the core pager.

xmacinfo’s picture

Status: Needs review » Closed (duplicate)

This is the duplicate!

yoroy’s picture

LewisNyman’s picture

Status: Closed (duplicate) » Active

After a quick discussion with yoroy on IRC, we came to the conclusion that we should keep the scope of these two issues separate.

We can reduce the scope of this issue to implement the new Seven styling to the existing pagers with no changes to mark up.

xmacinfo’s picture

This is a good news!

Theme and core needs to be kept separated.

LewisNyman’s picture

Issue tags: +CSS, +Novice, +CSS novice
frankbaele’s picture

Status: Active » Needs review
FileSize
1.18 KB

css patch without touching system.base.css

mordonez’s picture

Status: Needs review » Needs work
FileSize
32.86 KB

It seems necessary a bit more spacing on hover links

you can watch the review here https://saucelabs.com/tests/dfffc56bb14049f1a8d18a480b895f13

pager style

frankbaele’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

ok added some extra bottom padding to the focus & hover element

ckrina’s picture

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

Now working fine.

LewisNyman’s picture

Status: Reviewed & tested by the community » Needs work

We are not far off! Could we try reducing the font-weight of the first/previous/next/last links? They seem to little bit too strong, the current pager item isn't as prominent as the original designs.

From a code perspective, there's only a few improvements I could find:

+++ b/core/themes/seven/style.cssundefined
@@ -481,6 +481,47 @@ ul.inline li {
+  transition: all 0.2s;

We need to add a -webkit prefixed property here I think

+++ b/core/themes/seven/style.cssundefined
@@ -481,6 +481,47 @@ ul.inline li {
+  font-size: 14px;
...
+  line-height: 22px;

We should use ems here

ckrina’s picture

Assigned: Unassigned » ckrina

Starting to work on that.

ckrina’s picture

Assigned: ckrina » Unassigned
Status: Needs work » Needs review
FileSize
1.29 KB

Added both webkit and mozilla prefix.

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

The last submitted patch, new-pager-style-2016867-16.patch, failed testing.

rteijeiro’s picture

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

#16: new-pager-style-2016867-16.patch queued for re-testing.

rteijeiro’s picture

Issue summary: View changes

Updated issue summary.

LewisNyman’s picture

The patch applies well and the code looks good. Here's a before/after:

Screen Shot 2013-07-29 at 21.26.28.png

Screen Shot 2013-07-29 at 21.51.32.png

xmacinfo’s picture

Status: Needs review » Reviewed & tested by the community

@LewisNyman: Where you thinking of setting this to RTBC?

xjm’s picture

#16: new-pager-style-2016867-16.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, I need a stronger RTBC than that, from someone who's reviewed the CSS line-by-line and can vouch for it being correct.

Should this need a re-roll:

+++ b/core/themes/seven/style.css
@@ -481,6 +481,50 @@ ul.inline li {
+/**
+* Pagination.
+* The item-list CSS uses quite strong selectors, we have to match them here to override.
+*/

The starred lines under /** should be indented one space so they line up. And the comment should wrap at 80 characters. I can always fix it on commit, too.

xmacinfo’s picture

Status: Needs review » Needs work

We all can! :-)

LewisNyman’s picture

I've made a few minor modifications to the code and made the changes webchick mentioned.

LewisNyman’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Works as described in #2016867-19: Pager Style Update. It's a RTBC for me and looks shiny! :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

It looks like the existing CSS definition in seven's style.css can be removed.

.item-list .pager li {
  padding: 0.5em;
}
mcjim’s picture

Assigned: Unassigned » mcjim
mcjim’s picture

Status: Needs work » Needs review
FileSize
12.05 KB
997 bytes
1.51 KB

Rerolled patch to:

  • remove existing CSS definition (#27)
  • change the transition on hover from all to border-bottom-color after Lewis pointed that out
  • move the padding for the a element hover state only to the a element in all states to avoid any oddities with the transition (and as good practice, to stop the possibility of anything changing position as a result of changing states)
  • amend the padding bottom to 3px from 4px to line things up properly on the hover state

That looks more than it actually is :-)

mcjim’s picture

Assigned: mcjim » Unassigned

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

The last submitted patch, new-pager-style-2016867-28.patch, failed testing.

LewisNyman’s picture

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

#29: new-pager-style-2016867-28.patch queued for re-testing.

rteijeiro’s picture

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

Patch applies well and looks great! Maybe RTBC when testbot agree with it :P

pager.png

xmacinfo’s picture

Status: Reviewed & tested by the community » Needs work

Test failed.

LewisNyman’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Test is green.

yoroy’s picture

Status: Reviewed & tested by the community » Needs work

Looked this over with webchick and we're not seeing the underline on the active item in Firefox nor Chrome. Tested locally and on simplytest.me so there seems to be something missing in the patch.

mcjim’s picture

FileSize
192.81 KB

This is a screenshot of Chrome from a simplytest.me install.
Pager in Seven theme

(If you look closely at the bottom, you'll also see the pager from Barktik under the overlay. Unlike Seven, this doesn't have the underline for the current page.)

yoroy’s picture

Status: Needs work » Reviewed & tested by the community

Doh. And you specifically asked if we were looking at Seven. We weren't. Sorry! Back to rtbc.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a534266 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Added test page