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

[amp-pwa-reader] Structured Data is not shown on Structured Data Test Tool #82

Open
andreban opened this issue Jul 24, 2017 · 10 comments
Open
Assignees

Comments

@andreban
Copy link
Member

Structured Data Test Tool is unable to extract data from articles.

screen shot 2017-07-24 at 11 36 04 am

Results from SDTT can be seen here:
https://search.google.com/structured-data/testing-tool/u/0/#url=https%3A%2F%2Famp.cards%2Ftheguardian%2Fus%2Fus-news%2F2017%2Fjul%2F24%2Fjared-kushner-new-york-russia-money-laundering

@pbakaus
Copy link
Collaborator

pbakaus commented Jul 24, 2017

This most likely cannot be supported in this sample, as it is a purely client-side demo – unless the SDTT supports JS?

@sebastianbenz
Copy link
Collaborator

sebastianbenz commented Jul 24, 2017

Hm. Can we somehow fix this? Otherwise the sample is useless (or even worse misleading) for most news publishers and ecommerce devs. I'd be afraid that this leads to the misconception that PWAMP's don't support server-side rendering and are bad for SEO.

@pbakaus
Copy link
Collaborator

pbakaus commented Jul 24, 2017

We could fix this by deploying it on an actual server, with an actual backend, and use hybrid rendering. That should totally be the way to go for actual production apps. I don't have the bandwidth to look at this right now, but would be more than happy if somebody else could pick it up.

@sebastianbenz
Copy link
Collaborator

Why are we not using the amp-install-serviceworker based approach? First view is AMP which then installs the shell (at least for the article pages)?

@pbakaus
Copy link
Collaborator

pbakaus commented Jul 24, 2017

Yes, that would be the ideal setup. Again, requires a server-side component, since this uses proxied Guardian pages. Also don't think the Guardian would be super comfortable with us just directly proxying all their AMP pages (maybe).

@andreban
Copy link
Member Author

Could we add a section on the docs to make clear that this is not covered by the demo, and point to possible solutions.

@ithinkihaveacat
Copy link

@andreban The issue is mentioned in the Caveats section of the README, do you think it should be more prominent?

In at least one place the site is described as "production-ready" (albeit simultaneous with "sample")--that might not be quite the right word:

Meet the⚡️ ShadowReader, a production-ready PWA+AMP sample that uses real data from @guardian. Learn all about it 👇 https://t.co/jpPEPXLWLZ

— AMP Project (@amphtml) July 20, 2017

@andreban
Copy link
Member Author

There are 2 things that we can change on the current docs.

Search engines that don't understand JS won't be able to crawl the site (use hybrid rendering to address).

Even if the search engine supports Javascript, the Schema Data won't be available. We could append this information to the caveat.

amp-pwa-reader: Vanilla JS, detailed and SEO-friendly single-page progressive web app that displays AMP documents via Shadow DOM

I think the mentioned caveats make it hard to say that the project is entirely SEO-friendly. We may want to remove the claim from this part of the docs, and explain better what works and what doesn't work on the caveats.

@pbakaus
Copy link
Collaborator

pbakaus commented Jul 25, 2017

@andreban yes, both suggestions are fair. I'll take a look this afternoon and will amend the README.

@pbakaus
Copy link
Collaborator

pbakaus commented Aug 7, 2017

This requires a hybrid client/server rendering approach, which I've signed @amedina up to take a look at.

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

No branches or pull requests

5 participants