laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.13k stars 10.87k forks source link

Pagination issues with onEachSide #32943

Closed adwiv closed 4 years ago

adwiv commented 4 years ago

Description:

The default paginator appears to be optimized for onEachSide value of 3, resulting in discrepancies when onEachSide is set to a different value.

Issues with onEachSide set to 1

In order to make a small pagination views that fits in the mobile device view-port, I used the onEachSide() method with value 1 and observed the following issues:

  1. The number 2 is repeated on both sides separator when current page is 3 image

  2. The number (total pages - 1) is repeated when current page is (total pages - 2) image

There are some more superficial issues like

Issues with onEachSide set to 5

When onEachSide is set to 5, a different set of issues arise.

  1. The number of links on each side is not honored. For example, in the first image, the number of links on right side is only 2, and in the second image the links on left side are only 3. image image

Steps To Reproduce:

Create a pagination view for any query that returns more than 20 pages. {{ $query->paginate()->onEachSide(1)->links() }} {{ $query->paginate()->onEachSide(5)->links() }}

Suggestions/Bug Fixes

  1. Bug Fix: The class Illuminate\Pagination\UrlWindow seems to be optimized for default onEachSide value of 3. There are a lot of hardcoded assumptions that break when it is set to a value much different. For example, both the methods getSliderTooCloseToBeginning() and getSliderTooCloseToEnding() has a hard coded value of 2 in page range which should be equal t to $onEachSide and $onEachSide -1 respectively. Also, the value of $window in getUrlSlider() should ideally be $onEachSide + 4 instead of current value of $onEachSide * 2.

  2. Feature request: The paginator always shows 2 links on each end which results in minimum of 9 links being shown when onEachSide is set to 1. These many links can't fit in a xtra-small bootstrap screen. The suggestion is to either allow another setting onEachEnd on the paginator or assume the user wants a small paginator and show only one link on each end when onEachSide is set to 1.

driesvints commented 4 years ago

onEachSide shouldn't be used with anything lower than 2. I'll send in a PR to the docs to add a note about this.

Also see https://github.com/laravel/framework/issues/28789

adwiv commented 4 years ago

@driesvints That will still leave issues with onEachSide for values different than 2 or 3, for ex 5 as I have shown above. Only 2 links on the right when close to beginning and 3 links on left when close to ending are shown as these values are hard-coded.

Would you be interested in a very small and simple PR that introduces no breaking changes and passes all tests while fixing the paginator for all values from 1 onwards?

driesvints commented 4 years ago

@adwiv you can always attempt a PR but it's up to Taylor to decide if he wants it merged in.

taylorotwell commented 4 years ago

This will be fixed in the next patch release.

adwiv commented 4 years ago

Thanks a lot @taylorotwell for the patch.

There is a small change that I missed mentioning earlier.

When setting $window = $onEachSide + 4 we also need to change the condition when small slider is returned. The small slider should be returned whenever total pages is less than or equal to 2 + $onEachSide + 1 + $onEachSide + 2 i.e. last <= ($onEachSide * 2) + 7

Hence the condition in the UrlWindow::get() method should be changed from ($onEachSide * 2) + 6) to ($onEachSide * 2) + 8) in Line 47

With the current patch we are getting unnecessary separators in edge cases (when total pages = 12 or 13 for onEachSide set to default (3).

image image
forexgg commented 3 years ago

When this patch will be released to fix this? Not looking great using laravel6+bootstrap4 on cell phone. bootstrap4 has

shahind commented 2 years ago

Laravel 8 with Bootstrap 4.1 does not work as well.

Husky110 commented 2 years ago

Same with Laravel 9 and Bootstrap 4...

LukeTowers commented 2 years ago

See https://wintertricks.com/tricks/paginator-with-limited-number-of-pages-displayed-at-once for an example of how you can render a completely custom paginator link window setup (example is in Twig but is easily extrapolated to Blade or PHP.

denydias commented 2 years ago

I'm wondering why this one was closed as #28789 and its references provides no good solutions.

On Laravel 9.8.1 and Bootstrap 4.6.1, this is what paginator provides by default:

Screenshot_20220415_045408

So I came up with the following workaround:

<!-- On blade view (yeah, yeah... I know what they say about onEachSide() < 2 )-->
{!! $collection->onEachSide(1)->links() !!}
// Workaround Laravel Paginator Bad onEachSide #32943
// ---------------------------------------------------
@media (max-width: 575.98px) {
    .page-link {
        padding: 0.5rem 0;
        width: 1.85rem;
    }
}

Et voilà!

Screenshot_20220415_045433

It may still brake after hundreds pages tho, but still better than broken by default.

IMHO the perfect solution when onEachSide(1) would be (wich also extends to whatever > 1):

| < | 1 | ... | 8 | 9 | 10 | ... | 20 | > |

That clearly translates as 'one element on each side' developers expect while provides a predictable UX context:

No?

loeffel-io commented 8 months ago

Laravel 10 + Bootstrap 5 not working

AbiruzzamanMolla commented 7 months ago

Laravel 10 + Bootstrap 4 not working!