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

Remove microdata in html flow => replace per JSON-LD for classic theme #23042

Closed
wants to merge 0 commits into from

Conversation

fdonnet
Copy link
Contributor

@fdonnet fdonnet commented Jan 28, 2021

Questions Answers
Branch? develop
Description? Try to propose a solution for all the recurent issues with Google Rich Text validator.
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #22867
How to test? Fresh version of Presta, go on Goolge rich text validator, test index, category and product pages => clean
Possible impacts? Change in classic template. Add two new files included in head.tpl

The goal of this PR is to open the discussion about the microdata management.
A lot of issues are open about this topic recently.
The proposition is to keep the meta tags in the html flow but remove the microdata information and create a specific file that centralizes all the microdata and use JSON-LD for the declaration.
inspired a lot by github =>prestarocket-agence/classic-rocket

themes/classic/templates/_partials/microdata-jsonld.tpl :

This file is included in the head.tpl.
For the moment it's not perfect but it centralizes the info for futher improvements.

It takes care of this kind of microdata

  • Webpage
  • Organization
  • Search link
  • Category pages (ListItem but that only ref the product pages no product details in the ListItem)
  • Products (need to be standardized)
  • Product combinations declared as an offer for the product (with stock and correct price) => great stuff ;)
  • Breadcrumb

Edit : not included product ratings/review (need smarty var for combinations)

The product JSON declaration is not perfect (no EAN or other specific things). But like explained it's to open the discussion about the "centralization" of the concept and after that, maybe people who go deep;) will create new smarty variables that will help to enrich the microdata declaration. => For now, to have a product offer for each comination on the prouct page, it's great... I think.

I use this template on 2 shops and I can see a good amount of new "Google impressions" ... people find comibnations (colors, flavors) too. => solve the problem when you have single cannonical url per product and your combinations will be skipped by Google bot.

Other files => remove each instance of itemprop and itemscope
Trick for pagination pages

Hope it will help...


This change is Reviewable

@fdonnet fdonnet requested a review from a team as a code owner January 28, 2021 16:41
@prestonBot
Copy link
Collaborator

Hello @fdonnet!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Jan 28, 2021
@NeOMakinG
Copy link

hey @fdonnet

I appreciate your job, I created an issue in order to discuss a bit with the team about it ! #23046

Sanity are broken because they're based on itemprop attributes (wow), we'll need to change this because we'll never accept to mix itemprop and a separate file at the same time, if we choose to separate concerns, then let's go deeper :)

@fdonnet

This comment has been minimized.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 29, 2021

@fdonnet Hi! You can now put a proper mpn in the data, we have the field since 1.7.7. 😎

@fdonnet

This comment has been minimized.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 29, 2021

UPDATE 2: Here you go! #23055


UPDATE 1: Sorry, I added it to my Presta :D


@fdonnet Somebody removed it from $combinations variable. The EAN is there in 1.7.6, but it's missing in 1.7.7... I will make an issue.

"url": "{$item.url}"
}{if !$smarty.foreach.producttmp.last},{/if}
{/foreach}]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}
]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I think I m ok with the open and close "}" and "]" I don't understand your correction. :)

Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

Some feedback

@NeOMakinG
Copy link

Ok, thanks for following feedbacks

There are few more feedbacks about code styles, newlines, also, care about sanity tests (@boubkerbribri @nesrineabdmouleh, this PR changes a bit the markup, maybe we'll need to add selectors in order to avoid breaking tests, as you were using attributes selectors)

Also please, add new line at every file's end, and then I think we are almost good :)

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 3, 2021

Rebase for real "inital proposition", history of commits squashed because not very relevant (code cleaning, file separation).
Open discussion on product-jsonld.tpl

See, if we keep the product combinations declared as Offer or if we limit the scope to a simple Offer per product and let the shop owners override this template to fit their real needs (depending on their shop config => url rewrite etc) ?

At the end, the goal is to pass Google Rich text validator without (a lot) of warning to limit the creation of SEO issues in Github ;)... so maybe the combinations part in product-jsonld.tpl try to enhance a bit to much. (sad) ?

@Hlavtox
Copy link
Contributor

Hlavtox commented Feb 3, 2021

@fdonnet I think you can push the first part of changes without the combinations, so we have it sorted. We can then make proper research on how to approach this. I don't think it will consider it as duplicate, if it has proper canoncial. I will study it.

At the end, the goal is to pass Google Rich text validator without (a lot) of warning to limit the creation of SEO issues in Github ;)... so maybe the combinations part in product-jsonld.tpl try to enhance a bit to much. (sad) ?

No my friend, the goal is to make the best SEO, destroy the competition and squeeze the most money out of people. 😄

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 3, 2021

@Hlavtox

I don't think it will consider it as duplicate, if it has proper canoncial. I will study it.

Sure like you mentionned in a comment above, if we can have the $combination.url it can help to declare the combinations as offer. But not sure if it's sufficient

No my friend, the goal is to make the best SEO, destroy the competition and squeeze the most money out of people.

Héhé 👍 For sure, for the little shops I dev for, I will let the combinations part (because I have one url per product) and I already cross-checked that Google give more "impressions" with that and people find products the never found in the past. So it 's great.

But I don't want to create weird side effects for normal Prestashop base configurations.

Let see what is the opinion of others...

PS : UI Test failed => because of the itemprop removal (even with the new selectors)

@Hlavtox
Copy link
Contributor

Hlavtox commented Feb 4, 2021

@fdonnet @NeOMakinG Every resource I found on the internet just talks about one offer, and if the product has variants, they just say to add Aggregateoffers and specify min max price. Do we even have proper prices in $combinations?

They have separate offers here, but no URL??? 😄 https://www.schemaapp.com/newsletter/schema-org-variable-products-productmodels-offers/

IMHO, if you push it now without the combinations, it would be perfectly fine. And then you can just tweak that one file. WDYT?

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

hello @nesrineabdmouleh, maybe you or someone else can help me to fix the UI tests ?
Even with the new version merged it failed

on waiting for selector "#main h1" to be visible

(new selector)

. Strange because this tag is visible in my PR ?

@nesrineabdmouleh
Copy link
Contributor

Hello @fdonnet, Ok I will fix the UI tests problem.
But please fix the conflicting files!

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

@nesrineabdmouleh
Thx a lot

But please fix the conflicting files!

In my PR, I only removed all itemprop attributes (old UI selector) but apparently the new one is not working.
I don't know what I can modify to pass the test. (conflicting files ?).

@nesrineabdmouleh
Copy link
Contributor

@fdonnet, you're welcome!

@nesrineabdmouleh
Thx a lot

But please fix the conflicting files!

In my PR, I only removed all itemprop attributes (old UI selector) but apparently the new one is not working.
I don't know what I can modify to pass the test. (conflicting files ?).

Your branch has conflicts, you should resolve them before fixing UI tests.
-> themes/classic/templates/_partials/breadcrumb.tpl

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

@nesrineabdmouleh oh yes sry didn't see it, thx

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

@Hlavtox version without combination pushed, a little bit sad :) I think if we don't specify an url for combination it will raise a unnecessary Google warning. And we know how our lovely shop owners hate Google warnings even if it's not very relevant ;)

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

@nesrineabdmouleh Hello again
only UI tests / Sanity (7.4) (pull_request) failed don't know why, merge conflict fixed. ++

@nesrineabdmouleh
Copy link
Contributor

@nesrineabdmouleh Hello again
only UI tests / Sanity (7.4) (pull_request) failed don't know why, merge conflict fixed. ++

Hello @fdonnet, the error is not related to our tests, maybe there is a problem in FO with PHP 7.4
Can you test please?

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

@nesrineabdmouleh it seems linked to a selector : page.waitForSelector: Timeout 10000ms exceeded. ? strange ?

@nesrineabdmouleh
Copy link
Contributor

@nesrineabdmouleh it seems linked to a selector : page.waitForSelector: Timeout 10000ms exceeded. ? strange ?

It cannot find the selector of Product name on product FO page, maybe there is a problem in the page.
The test is passed with PHP 7.1 - 7.2 and 7.3

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

@NeOMakinG hi, maybe you can help us on that (php7.4 test failed) ?

@boubkerbribri
Copy link
Contributor

boubkerbribri commented Feb 4, 2021

Hi @fdonnet, please check the screenshot of the error https://github.com/PrestaShop/PrestaShop/suites/1966830440/artifacts/39072336

I think it's related to your PR.

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 4, 2021

@boubkerbribri thx, i will investigate tomorrow ;)

@fdonnet
Copy link
Contributor Author

fdonnet commented Feb 5, 2021

PLS SEE => #23151
Mega pro git command guy !

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

Successfully merging this pull request may close these issues.

Move structured data to a separate file
8 participants