Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.3] Hide empty paginators #15125

Merged
merged 1 commit into from
Aug 31, 2016
Merged

Conversation

garygreen
Copy link
Contributor

Fixes #15124

@taylorotwell
Copy link
Member

What is $elements and where does that variable come from?

@taylorotwell
Copy link
Member

"Elements" is only passed by the length aware paginator.

<!-- "Three Dots" Separator -->
@if (is_string($element))
<li class="page-item disabled"><span class="page-link">{{ $element }}</span></li>
@if (count($elements) > 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just just ask the paginator how large it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

count($paginator)

@garygreen garygreen force-pushed the hide-empty-paginator branch 2 times, most recently from 5fae59b to 2a3a723 Compare August 30, 2016 15:52
@garygreen
Copy link
Contributor Author

Not sure why I made that more complicated than it needs to be. I've updated PR to just check for ->count() on the paginator @taylorotwell @GrahamCampbell 😄

@taylorotwell taylorotwell merged commit e9d5846 into laravel:5.3 Aug 31, 2016
@nhowell
Copy link
Contributor

nhowell commented Sep 2, 2016

It still renders when there is only one page but there is more than one item to display, which is different from 5.2.

@GrahamCampbell
Copy link
Member

Meh, that's ok I think. Kinda odd to hide in that case.

@garygreen
Copy link
Contributor Author

Er so that's why I had the code as I did... I wasn't going mad! I think it should hide it imo, if there's only one page then no point displaying pagination.

@nhowell
Copy link
Contributor

nhowell commented Sep 2, 2016

@GrahamCampbell I don't think it's ok to not render when there's only one item on the page, while still rendering when there are two items on the page. That's just plain inconsistent.

@nhowell
Copy link
Contributor

nhowell commented Sep 2, 2016

I would prefer that it doesn't render the pagination links at all unless there is more than one page. Plus, it would make it consistent with 5.2.

But if that's too much trouble, we should at least render the pagination links when there is only one item on the page as well:

// Change this:
@if ($paginator->count() > 1)

// To this:
@if (! $paginator->isEmpty())

@GrahamCampbell
Copy link
Member

Oh right I see what you mean. This code should be > 0 not > 1. Please PR. :)

@nhowell
Copy link
Contributor

nhowell commented Sep 2, 2016

Oh, and I just realized there is another issue with the current code.

Say you used have 26 items and you $items->paginate(25). The first page has 25 items, while the second page will only have one item and therefore the pagination links will be hidden on the second page.

@garygreen
Copy link
Contributor Author

I think the code should be what I had it as before....

For the non simple paginators it should just be:

@if (count($elements))

Then for the simple paginators it should be...

@if (!$paginator->onFirstPage() || $paginator->hasMorePages())

@nhowell
Copy link
Contributor

nhowell commented Sep 2, 2016

@garygreen Oh, I like that.

And in fact, couldn't we use @if (!$paginator->onFirstPage() || $paginator->hasMorePages()) for both types of paginators? I think that would actually retain the same behavior of 5.2 by not rendering at all unless there is more than one page.

@taylorotwell
Copy link
Member

Reverted. Please send a correct PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants