Skip to content

add canonical pagination links#1301

Open
JimmyHoenderdaal wants to merge 2 commits into
masterfrom
feature/canonical-pagination
Open

add canonical pagination links#1301
JimmyHoenderdaal wants to merge 2 commits into
masterfrom
feature/canonical-pagination

Conversation

@JimmyHoenderdaal

Copy link
Copy Markdown
Member

Description
Adds canonical, prev, and next links for paginated category pages. This helps search engines understand paginated listing URLs and avoids duplicate content issues.

Breadcrumb structured data was already present through BreadcrumbList.

Tests

Checked rendered head tags on rapidez.test/women/tops-women.html
Checked rendered head tags on ?page=1 and ?page=2
Ran PHP syntax checks

Reference
RAP-1898

@@ -0,0 +1,17 @@
@php
$currentPage = max((int) request('page', 0), 0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the minimum value for page be 1 and not 0?

Suggested change
$currentPage = max((int) request('page', 0), 0);
$currentPage = max((int) request('page', 1), 1);

@@ -0,0 +1,17 @@
@php
$currentPage = max((int) request('page', 0), 0);
$lastPage = max((int) ceil($total / max((int) $perPage, 1)) - 1, 0);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given that we always define what $perPage is when using this component, and ceil will always return a float, we don't need to do so many casts and extra checks.

Doing the -1 also doesn't make sense if pages are 1-indexed. We now have an off-by-one error. (you can check this yourself by assuming $perPage is 2: if you have 5 products, you should have 3 pages, and ceil(5/2) = 3 already.)

Suggested change
$lastPage = max((int) ceil($total / max((int) $perPage, 1)) - 1, 0);
$lastPage = max(ceil($total / $perPage), 1);

@php
$currentPage = max((int) request('page', 0), 0);
$lastPage = max((int) ceil($total / max((int) $perPage, 1)) - 1, 0);
$pageUrl = fn (int $page) => $page > 0 ? $url . '?' . http_build_query(['page' => $page]) : $url;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems unnecessary. Aside from the fact that, again, pages are 1-indexed so this will never return false in the first place, you can just have ?page=1 without any issues. I'd remove this line and keep it simple with the lower two lines using something like

href={{ $url . '?page=' . $currentPage - 1 }}

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.

2 participants