Disable buttons for pagination

[batmoo]

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

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

[batmoo]

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

[JohnONolan]

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.

[garyc40]

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

[garyc40]

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

[garyc40]

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

[westi]

• Milestone changed from Future Release to 3.1

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

[nacin]

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

[nacin]

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.

[nacin]

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

[nacin]

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

[garyc40]

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?

[nacin]

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.

[garyc40]

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.

[nacin]

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.

[garyc40]

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

[garyc40]

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

[garyc40]

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.

[nacin]

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

[nacin]

Updated against [17206]

[nacin]

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.

[garyc40]

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.

[koopersmith]

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.

[nacin]

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