WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#15503 closed enhancement (fixed)

Disable buttons for pagination

Reported by: batmoo Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch needs-ui 3.2-early
Focuses: Cc:

Description

When viewing the Page 1, the previous and first buttons should be disabled. Similarly, when viewing the last page, the next and last buttons should be disabled.

Related #14579

Attachments (7)

15503.diff (3.5 KB) - added by batmoo 5 years ago.
garyc40-15503.patch (7.1 KB) - added by garyc40 4 years ago.
disable pagination buttons using span, also properly update href attributes when changing pages
garyc40-15503-rev2.patch (7.9 KB) - added by garyc40 4 years ago.
fixed some bugs introduced in rev1, also fixed disabled span's styles so that pagination links do not jump around
15503.2.diff (4.4 KB) - added by nacin 4 years ago.
15503.3.diff (1.8 KB) - added by nacin 4 years ago.
15503.4.patch (2.4 KB) - added by garyc40 4 years ago.
fixed a typo in class-wp-list-table.php
15503.5.diff (1.8 KB) - added by nacin 4 years ago.
Updated against [17206]

Download all attachments as: .zip

Change History (35)

@batmoo5 years ago

comment:1 @PeteMall5 years ago

  • Keywords ui-feedback added; ui pagination removed
  • Milestone changed from Awaiting Review to 3.1

comment:2 follow-up: @JohnONolan5 years ago

  • Keywords needs-patch needs-ui added; has-patch ui-feedback removed

This patch is styling a link to look like it's disabled when it's in fact still a link, I don't think that's really how we want to go about this.

comment:3 in reply to: ↑ 2 @batmoo5 years ago

Replying to JohnONolan:

This patch is styling a link to look like it's disabled when it's in fact still a link, I don't think that's really how we want to go about this.

Ah, you're right, especially doesn't make sense in a no-js context. Is it better to hide the buttons instead? Or better to use a different tag (e.g. a span)?

comment:4 @JohnONolan5 years ago

Go for a different tag (span works) - if possible don't introduce a new class, eg. if you think that

.tablenav-pages span

would work because the only time a span will ever be used is for an inactive pagination menu item - then go for that.

comment:5 @JohnONolan5 years ago

  • Milestone changed from 3.1 to Future Release

comment:6 @JohnONolan5 years ago

  • Keywords 3.2-early added
  • Type changed from defect (bug) to enhancement

@garyc404 years ago

disable pagination buttons using span, also properly update href attributes when changing pages

comment:7 @garyc404 years ago

  • Keywords has-patch added; needs-patch removed

comment:8 @garyc404 years ago

I had to introduce a new class called "disabled". Otherwise .tablenav-pages span alone would also select the "xxx items" span.

comment:9 @nacin4 years ago

  • Owner changed from batmoo to nacin
  • Status changed from new to accepted

comment:10 @nacin4 years ago

  • Milestone changed from Future Release to 3.1

@garyc404 years ago

fixed some bugs introduced in rev1, also fixed disabled span's styles so that pagination links do not jump around

comment:11 @westi4 years ago

  • Milestone changed from Future Release to 3.1

Why is this super critical - the feature works fine - this is cake icing ;-)

comment:12 @nacin4 years ago

#16056 was closed as a duplicate. It's a bad user experience. I'm working up a very conservative approach for 3.1 consideration.

comment:13 @nacin4 years ago

I don't like the span/a toggle here. Just stick to links and make the links noop. I'm just not interested in avoiding an anchor in non-JS situations -- it makes the code too convoluted.

I've patched this using a minimal amount of code, but I think I've picked up on the same bug gary did, "also properly update href attributes when changing pages", which I can see in a browser that doesn't support pushState (i.e. Firefox 3.6.13). Basically, the next link reflects paged=2 even though I'm on page 38 of 39. The link does take me to page 39 though.

I'm committing my fix for now as it isn't a regression. We might need some massaging to fix that related bug, if it is indeed an issue we should be concerned with. Minimal code churn is important.

@nacin4 years ago

comment:14 @nacin4 years ago

(In [17202]) Disabled states for first/prev and next/last pagination buttons when they should noop. see #15503.

comment:15 @nacin4 years ago

(In [17203]) Compress and bump scripts for [17202]. see #15503.

comment:16 @garyc404 years ago

I have to disagree with nacin about keeping the links and only no-op them instead of switching to span. It's basically fixing a bad experience by introducing a less seriously bad experience (e.g. When no-JS, tab navigating etc.)

Since when do we settle for compromise instead of fixing the root cause and keeping the semantics clean?

Sure, the code might be a bit convoluted. But that's what it takes to get the job done, unless somebody comes up with a more elegant approach to toggle span / link. CSS changes for :focus and :hover is hardly anything more than duck tape.

I'd still prefer to keep this out of 3.1 until we have some more discussion and feedback from ux. Or perhaps I'm just a perfectionist when it comes to tiny little details like this.

Thoughts?

comment:17 @nacin4 years ago

Regardless, the patches proposed were too much for 3.1, and we needed something. I have no problem revisiting in 3.2.

My comment above, http://core.trac.wordpress.org/ticket/15503#comment:13, mentions another bug I think you noticed. Think you could try to patch that one, in the context of 3.1? Does it even fix anything practical? Things worked fine with pushState supported, pushState unsupported, and JS off -- all it seems to fix is the value of the href itself, perhaps preventing a control-click to open that page in a new window? I'm not even sure.

comment:18 @garyc404 years ago

Sorry, I didn't pay enough attention and thought you closed the ticket as fixed.

The url thing only affects the links' href attributes, not that much of a big deal. I personally usually command-click on those links, so it might be a problem to a few other people. It'd still be nice if it were fixed.

I'll do a patch in a couple of hours or so. It would look like updatePageLinks() function in one of my patches above.

@nacin4 years ago

comment:19 @nacin4 years ago

Attached patch seems to handle URL updates just fine.

Also fixes a small bug in [17202], where I used total_pages_i18n instead of total_pages.

comment:20 @garyc404 years ago

Hang on, found a bug in class-wp-list-table.php :) just one min.

@garyc404 years ago

fixed a typo in class-wp-list-table.php

comment:21 @garyc404 years ago

The only difference is probably that I don't update href of disabled links as well as that of the first-page link, since it's not necessary.

Also, fixed a typo in class-wp-list-table.php . I'm sure you meant $total_pages instead of $total_items.

comment:22 @nacin4 years ago

(In [17206]) Use correct variables. props garyc40, see #15503.

@nacin4 years ago

Updated against [17206]

comment:23 @nacin4 years ago

Got the typos out of the way in r17206, thanks. Refreshed 15503.5.diff.

The only difference is probably that I don't update href of disabled links as well as that of the first-page link, since it's not necessary.

I did think about that, but $.query.set() actually updates the entire URL, thus any other filtering, sorting, or searching. Thus it was nice to still update the first-page link, and after that, I decided not to bother excluding the disabled links.

comment:24 @garyc404 years ago

Fair enough. I guess this is working well for 3.1 :)

It still bothers me a bit when the URLs of the disabled links pop up in the status bar on :hover, but hopefully we'll have time to come back and fix it in 3.2.

comment:25 @koopersmith4 years ago

The span/a change seems a little bloated for such a minor UX issue; manipulating the DOM shouldn't be necessary here.

Similarly, I agree with Nacin that the final patch isn't a practical fix. There are a handful of potential options for a relatively trivial issue (for instance, we could clear the unused href once the DOM has loaded). Let's review this further in 3.2.

comment:26 @nacin4 years ago

  • Milestone changed from 3.1 to Future Release

comment:27 @nacin3 years ago

  • Owner nacin deleted
  • Status changed from accepted to assigned

comment:28 @nacin3 years ago

  • Milestone changed from Future Release to 3.1
  • Resolution set to fixed
  • Status changed from assigned to closed

Since ajax list tables were pulled, we can close this as fixed on 3.1.

Note: See TracTickets for help on using tickets.