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

Show the category block in the list page on first page only #9459

Merged
merged 2 commits into from Sep 6, 2018

Conversation

Projects
None yet
6 participants
@jolelievre
Contributor

jolelievre commented Aug 16, 2018

Questions Answers
Branch? develop
Description? Show the category block in the list page on first page only for SEO purposes
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #9512
How to test? Go to a category with several pages (if needed change the number of displayed product in Admin / Shop Parameters / Product Settings), go to seconde page using the url (a simple click updates the current page thanks to ajax calls), the category bloc is not present any more WARNING: be sure to clear your cache so that the Javascript is updated

This change is Reviewable

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Aug 30, 2018

Contributor

@eternoendless hello
I made the modification we talked about with the new id directly on the h2 tag
Is it ok for you?

Contributor

jolelievre commented Aug 30, 2018

@eternoendless hello
I made the modification we talked about with the new id directly on the h2 tag
Is it ok for you?

jolelievre added some commits Aug 16, 2018

Show the category block in the list page on first page only for SEO p…
…urpose, change ajax update to match the behaviour
@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Sep 5, 2018

Contributor

@eternoendless hell PR rebase and assets rebuilt, can you validate it please?

Contributor

jolelievre commented Sep 5, 2018

@eternoendless hell PR rebase and assets rebuilt, can you validate it please?

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Sep 5, 2018

Contributor

thanks @PierreRambaud
I wait for the build succeed before merging

Contributor

jolelievre commented Sep 5, 2018

thanks @PierreRambaud
I wait for the build succeed before merging

@marionf marionf self-assigned this Sep 6, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Sep 6, 2018

@marionf marionf removed their assignment Sep 6, 2018

@eternoendless

Reviewed 4 of 6 files at r1, 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


controllers/front/listing/CategoryController.php, line 145 at r1 (raw file):

    {
        $data = parent::getAjaxProductSearchVariables();
        $rendered_products_header = $this->render('catalog/_partials/category-header', array('listing' => $data));

Keep in mind that 3rd party themes (not based in classic) won't have this partial initially. What happens in that case?

@eternoendless

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jolelievre)

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre

jolelievre Sep 6, 2018

Contributor

@eternoendless in another theme the user won't have this feature available, the render in ajax will fail silently (returning an empty string), and the id allowing to replace the content won't exist
so no BC breaks, waiting for your review acceptance to merge ;)

Contributor

jolelievre commented Sep 6, 2018

@eternoendless in another theme the user won't have this feature available, the render in ajax will fail silently (returning an empty string), and the id allowing to replace the content won't exist
so no BC breaks, waiting for your review acceptance to merge ;)

@eternoendless

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Sep 6, 2018

Thank you @jolelievre

@eternoendless eternoendless merged commit d4c990a into PrestaShop:develop Sep 6, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
code-review/reviewable 7 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jolelievre jolelievre deleted the jolelievre:BOOM-6248 branch Sep 6, 2018

@rdy4ever

This comment has been minimized.

Show comment
Hide comment
@rdy4ever

rdy4ever Sep 18, 2018

Contributor

Ok. May I ask "why?". Google knows what a header is and we're talking here about a listing page with pagination. Google also knows that a header element on each page of a listing is not duplicate content. Wouldn't it be much simple just to mark the category header block as <header>?
Also, category pages already have the link rel="canonical" set, so Google understands that the subsequent pages are part of the main category page.
Plus, with this PR all the other then first pages would miss the H1 tag.
To me this looks like unnecessary added complexity.

Contributor

rdy4ever commented Sep 18, 2018

Ok. May I ask "why?". Google knows what a header is and we're talking here about a listing page with pagination. Google also knows that a header element on each page of a listing is not duplicate content. Wouldn't it be much simple just to mark the category header block as <header>?
Also, category pages already have the link rel="canonical" set, so Google understands that the subsequent pages are part of the main category page.
Plus, with this PR all the other then first pages would miss the H1 tag.
To me this looks like unnecessary added complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment