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 and replace by JSON-LD for classic theme #23151

Merged
merged 15 commits into from Mar 24, 2021

Conversation

fdonnet
Copy link
Contributor

@fdonnet fdonnet commented Feb 5, 2021

Questions Answers
Branch? develop
Description? Try to propose a solution for all the recurent issues with Google Rich Text validator. (REOPEN => I made a silly git mistake I was not able to roll back)
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 ;) Not implemented for standard shop (people will be able to override)
  • Breadcrumb

The product JSON declaration is not perfect. 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.

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. (available in the first commit of this PR, but removed to remain standard)

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 February 5, 2021 09:27
@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!

@fdonnet

This comment has been minimized.

@fdonnet

This comment has been minimized.

@PierreRambaud PierreRambaud changed the title (REOPEN) Remove microdata in html flow => replace per JSON-LD for classic theme Remove microdata in html flow => replace per JSON-LD for classic theme Feb 5, 2021
@boubkerbribri
Copy link
Contributor

Hello @fdonnet ,
I'm sorry but me and @nesrineabdmouleh can't help you fix this error because it's not related to the tests, maybe @PrestaShop/prestashop-maintainers

What I can assure you is that I checkout you PR and the error exists on php7.4
image

@fdonnet

This comment has been minimized.

@fdonnet

This comment has been minimized.

@fdonnet

This comment has been minimized.

@NeOMakinG
Copy link

I can't get why it would fail on 7.4 and not on previous versions, this is pretty strange

@NeOMakinG NeOMakinG closed this Feb 11, 2021
@NeOMakinG NeOMakinG reopened this Feb 11, 2021
@NeOMakinG
Copy link

Running tests again as the fail isn't from the PR

@NeOMakinG NeOMakinG closed this Feb 11, 2021
@NeOMakinG NeOMakinG reopened this Feb 11, 2021
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.

A small feedback

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.

A last one

Progi1984
Progi1984 previously approved these changes Feb 16, 2021
@fdonnet

This comment has been minimized.

@fdonnet fdonnet dismissed stale reviews from Progi1984 and kpodemski via d40c7e2 March 24, 2021 13:50
Copy link

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

I tested with the fix:
https://search.google.com/test/rich-results?id=d-gSZPtkd3p4T5GjTKImNg

It's fine now (errors are generated because my urls aren't reachable by Google)

@NeOMakinG NeOMakinG added QA ✔️ Status: check done, code approved and removed Waiting for author Status: action required, waiting for author feedback labels Mar 24, 2021
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@PrestaShop PrestaShop deleted a comment from prestashop-issue-bot bot Mar 24, 2021
@NeOMakinG NeOMakinG added this to the 1.7.8.0 milestone Mar 24, 2021
@NeOMakinG
Copy link

As it's a QA by dev, feel free to merge it when it gets 2 approves!

Good job @fdonnet

@fdonnet
Copy link
Contributor Author

fdonnet commented Mar 24, 2021

Thx, @NeOMakinG for the reviews, the advice and the support !

@NeOMakinG
Copy link

Trying to run tests again

@NeOMakinG NeOMakinG closed this Mar 24, 2021
@NeOMakinG NeOMakinG reopened this Mar 24, 2021
@Progi1984 Progi1984 merged commit 89fc139 into PrestaShop:develop Mar 24, 2021
@prestonBot
Copy link
Collaborator

Contribution merged, congratulations!

Would you mind answering our quick 1-minute survey? We would love to hear about your experience so far, it will help us improve our process for the community involved, like you. ;-)

@Progi1984
Copy link
Contributor

Thanks @fdonnet & @NeOMakinG for QA ;)

@matks matks added the Key feature Notable feature to be highlighted label Mar 24, 2021
@prestashop-issue-bot
Copy link

QA OK without required approvals !? :trollface:

@matks matks changed the title Remove microdata in html flow => replace per JSON-LD for classic theme Remove microdata in html flow and replace by JSON-LD for classic theme Mar 24, 2021
@Hlavtox
Copy link
Contributor

Hlavtox commented Mar 24, 2021

@fdonnet You're the boss!!!

@fdonnet
Copy link
Contributor Author

fdonnet commented Mar 24, 2021

@Hlavtox thx for your help too on polishing a lot of things ! Cool to be merged ;)

@Progi1984
Copy link
Contributor

@fdonnet @Hlavtox Good job guys 💪

"url" : "{$urls.pages.index}",
"image": {
"@type": "ImageObject",
"url":"{$urls.shop_domain_url}{$shop.logo}"

Choose a reason for hiding this comment

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

hello, this is bad
{$urls.shop_domain_url}{$shop.logo} --> {$shop.logo}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgielecinski already modified in d40c7e2

Choose a reason for hiding this comment

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

in organization yes, but in line 60 still is error

Copy link
Contributor Author

@fdonnet fdonnet Apr 27, 2021

Choose a reason for hiding this comment

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

@mgielecinski oh yeah I see :) Thx a lot. @Progi1984 for info I opened #24239 (pull request) to correct this based on mgielecinski laser eyes recommendation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement Key feature Notable feature to be highlighted QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move structured data to a separate file