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

Fix creating pager range #245

Merged
merged 15 commits into from
May 3, 2018
Merged

Fix creating pager range #245

merged 15 commits into from
May 3, 2018

Conversation

serieznyi
Copy link

@serieznyi serieznyi commented Nov 21, 2017

Pager always create count pages equals value of PagerAwareViewData::maxPages.
So I am fix it.
I use logic from this function in repository jasongrimes/php-paginator

UPDATE 1
I use modified code of @JanTvrdik

@JanTvrdik
Copy link

This BTW what we use

public function getPages(): array
{
	$start = 1;
	$numAdjacents = (int) floor(($this->maxPages - 1) / 2);

	if ($this->currentPage - $numAdjacents < $start) {
		$begin = $start;
		$end = min($this->numPages, $this->maxPages);

	} elseif ($this->currentPage + $numAdjacents > $this->numPages) {
		$begin = max($start, $this->numPages - $this->maxPages + 1);
		$end = $this->numPages;

	} else {
		$begin = $this->currentPage - $numAdjacents;
		$end = $this->currentPage + $numAdjacents;
	}

	return range($begin, $end, 1);
}

@serieznyi serieznyi changed the base branch from master to 2.1 November 22, 2017 06:11
@serieznyi
Copy link
Author

serieznyi commented Nov 22, 2017

@JanTvrdik Yours variant more shortly ))

@serieznyi
Copy link
Author

serieznyi commented Nov 22, 2017

@JanTvrdik I try your code. It is good.
By I think we need always show first and last pages.
So for this we need little change last part of code

return array_unique(array_merge(
            [1],
            range($begin, $end, 1),
            [$this->numPages]
        ));

and we get that

image

I try use your code in PR

@serieznyi serieznyi changed the base branch from 2.1 to master November 22, 2017 06:36
@serieznyi
Copy link
Author

I think this changes need be in 2.1 but my branch based on master
My wrong.
I can recreate branch later if this changes be merged

@saimaz saimaz changed the base branch from master to 2.1 November 22, 2017 08:24
@saimaz
Copy link
Member

saimaz commented Nov 22, 2017

Looks like a good fix. Please rebase you PR to 2.1 branch. I have changed the base in the PR, you only need to rebase and push the changes.

@JanTvrdik
Copy link

@serieznyi We solve the „always display first page“ problem in template

{% set paginationRoute = app.request.attributes.get('_route') %}
{% set paginationParams = filters.pagination.getUrlParameters() %}
{% set pages = filters.pagination.pages %}

<ul>
	{% if pages|first != 1 %}
		<li><a href="{{ ongr_paginate_path(paginationRoute, 1, paginationParams) }}">1</a></li>
		<li>&hellip;</li>
		{% set pages = pages|slice(2) %}
	{% endif %}

	{% for page in pages %}
		<li class="{% if page == filters.pagination.getCurrentPage %}is-active{% endif %}">
			<a href="{{ ongr_paginate_path(paginationRoute, page, paginationParams) }}">{{ page }}</a>
		</li>
	{% endfor %}
</ul>

(cherry picked from commit e261db6)
(cherry picked from commit 99436de)
(cherry picked from commit 2179b86)
@serieznyi
Copy link
Author

@saimaz with rebase my branch contains not may commits. So i do cherry-pick.
Also you need what while I check test.
For some reason tests not run in travis

@serieznyi
Copy link
Author

@saimaz why travic-ci fail on prepare tests environment? What I need to do?

@serieznyi
Copy link
Author

@saimaz I read Travic-CI docs and understand what a problem.
You need update or set GITHUB_COMPOSER_AUTH env variable on https://travis-ci.org/ongr-io/FilterManagerBundle/settings page

@saimaz
Copy link
Member

saimaz commented Dec 2, 2017

Thanks @serieznyi for the changes. I fixed Travis issues. Lets see if there will be any errors.

@saimaz
Copy link
Member

saimaz commented Dec 2, 2017

Ok, seems like token fix didn't help. There is no java 8 anymore in the current travis image we are using. So I changed the java version to 9 in the 2.1 branch. Apparently one test is failing, I will take a look whats wrong with it. If someone could help on this I would appreciate.

@serieznyi
Copy link
Author

serieznyi commented Dec 3, 2017

@saimaz I create issue and PR for problem with tests. I be wait when you merge tests fix and when I merge 2.1 with new changes in current PR

@saimaz
Copy link
Member

saimaz commented Dec 3, 2017

Thanks @serieznyi for the fixes it helped me a lot! Appreciate this. You can rebase now with 2.1 branch and I could merge this as well and then release a patch version.

@serieznyi
Copy link
Author

@saimaz maybe i simple merge origin/2.1 in my feature branch and you use squash merge if all ok?

@saimaz
Copy link
Member

saimaz commented Dec 3, 2017

Sure I could squash merging the PR, but travis still freaks out with errors :(

@serieznyi
Copy link
Author

serieznyi commented Dec 3, 2017

@saimaz I know about tests fail. I try fix it in few days

@serieznyi
Copy link
Author

@saimaz tests fixed. I think what you need see my code and analyse what I do.

@saimaz
Copy link
Member

saimaz commented Dec 5, 2017

Why did you set max pages to 3 instead of 2?

@serieznyi
Copy link
Author

@saimaz Because I throw exception then maxPages less 3.
Why? Because I always return first and last pages from getPages method. And if user set maxPages equal 2 or less then he see only first and last page.

Also I have some errors in my logic and I try fix it.

@serieznyi
Copy link
Author

serieznyi commented Jan 12, 2018

@saimaz I fix my logic. You can check it. Try tests

$this->numPages = (int) ceil($this->totalItems/$this->itemsPerPage);

$this->maxPages = $maxPages < 3 ? 3 : $maxPages;
Copy link
Author

@serieznyi serieznyi Jan 12, 2018

Choose a reason for hiding this comment

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

@saimaz I replace throwing exception on default minimum value because I afraid that my changes crash somebody app.

@serieznyi
Copy link
Author

serieznyi commented Feb 23, 2018

Anybody can watch my PR? Please )

@saimaz
Copy link
Member

saimaz commented Mar 20, 2018

I'm checking it. Looks quite good. Will test it and merge it quite soon ;) Thanks for the PR!

@11mb
Copy link

11mb commented May 3, 2018

quite soon? ;)

@saimaz saimaz merged commit ab879a9 into ongr-io:2.1 May 3, 2018
@saimaz
Copy link
Member

saimaz commented May 3, 2018

yep ;)

@khusseini
Copy link

How come this was only merges into 2.1? Isn't this bug relevant for all branches?

@serieznyi serieznyi deleted the fix-pager branch September 27, 2018 21:57
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.

5 participants