See the attached snapshots, the pager links looks much closer and makes it difficult to click appropriate link.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

I don't know... hmmm

james.elliott’s picture

+1 to the extra spacing

yoroy’s picture

I'd much rather see work put into #538788: Implement Seven pagers as designed

rickvug’s picture

Status: Needs review » Needs work

Is modifying the css reset the best place to do this? I would think that the change should go into style.css. There is an argument to be made that the reset is overly aggressive, but that should be dealt with at #723392: Tame seven's reset.css.

As for the change itself, I think that it is a welcome improvement. The spacing is maybe 2-5px too wide for my liking but I'd be bike shedding to request a change. :) Let's try to get this simple change in now in case #538788: Implement Seven pagers as designed doesn't make it.

Sivaji_Ganesh_Jojodae’s picture

@yoroy we are too late read Bojhan's comment #24

@rickvug Adding all selector to reset.css is not a good way either. This is addressed in #723392: Tame seven's reset.css so this issue will be fixed when #723392 is committed.

corbacho’s picture

I like this change and we are late to make it "as designed", so maybe this patch should be re-considered.

It's difficult to click correctly on the numbers. BUT maybe not so wide. Too much for my liking (as rickvug said)

Jeff Burnz’s picture

Status: Needs work » Needs review

seven-pager.patch queued for re-testing.

droplet’s picture

Version: 7.x-dev » 8.x-dev
droplet’s picture

Issue tags: -Usability, -Novice

seven-pager.patch queued for re-testing.

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

The last submitted patch, seven-pager.patch, failed testing.

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
671 bytes

I added

+.item-list .pager li {
+  padding: 0.5em;
+}

As in Bartik theme.

Mac_Weber’s picture

result's screenshot
Seven theme's pager spacing

afeijo’s picture

Status: Needs review » Reviewed & tested by the community

it worked, congrats

Mac_Weber’s picture

It works in both 7.x and 8.x

Mac_Weber’s picture

Issue tags: +Needs backport to D7
dcrocks’s picture

What does this look like to someone who normally runs their browser at 18pt?

yoroy’s picture

dcrocks, why do you ask? Your question lacks info on why that is important to check.
Not sure on the need (possibility) to backport this but looks good, fixes things in the right place. Thanks for the work and screenshots Mac_Weber.

dcrocks’s picture

It does look nice. But, since he uses 'em's for spacing will it look nice at larger fonts? It may not be a big deal here, but the UI has to work for everyone and these things should be considered. Don't stop this because of me, but permit the question.

webchick’s picture

dcrocks: Can you test it and let us know how it works, rather than asking someone else to? :)

dcrocks’s picture

FileSize
7.63 KB
7.14 KB
6.68 KB

First I had to find something on my test site that actually showed a pager. Found the system log report. The samples are firefox on a mac.
#1 no patch, 16pt font default
#2 patch, 16pt font
#3 patch, 18pt font

Still looks OK to me but some might dislike the whitespace. The only possible problem is if pager displayed in a fixed width block the pager object could unexpectedly overflow with larger fonts.

droplet’s picture

Remind: system module is using EM. Please also submit a patch for it if it need to be PX.

Jeff Burnz’s picture

General rule of thumb is if the text can scale 200% and nothing breaks then its good. ems should be fine - as long as the pager still works OK when it wraps, it good also.

Mac_Weber’s picture

@dcrocks you can always use drush+devel to generate content. Thus you would get a pager on your content management page

drush dl devel && drush en devel_ge* -y && drush genc 200

webchick’s picture

Status: Reviewed & tested by the community » Fixed

The screenshots at #20 look fine to me, sounds like others are in agreement as well.

Committed and pushed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)

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