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

Add AMP #1205

Merged
merged 16 commits into from
Dec 18, 2017
Merged

Add AMP #1205

merged 16 commits into from
Dec 18, 2017

Conversation

Sekhmet
Copy link
Contributor

@Sekhmet Sekhmet commented Dec 16, 2017

Fixes #1146

Changes:

  • Add AMP template.
  • Add ampRender.
  • Prepare content for AMP (replace images, remove head, select content inside body).
  • Link pages using canonical URLs.
  • Pass images to schema.
  • Extract renderers.
  • Extract handlers.

@Sekhmet Sekhmet changed the title Amp Add AMP Dec 16, 2017
@bonustrack bonustrack temporarily deployed to busy-master-pr-1205 December 16, 2017 13:35 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1205 December 16, 2017 16:32 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1205 December 16, 2017 16:54 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1205 December 16, 2017 17:25 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1205 December 16, 2017 17:40 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1205 December 16, 2017 17:51 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1205 December 16, 2017 18:55 Inactive
@Sekhmet Sekhmet removed the PR: WIP label Dec 16, 2017
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1205 December 16, 2017 20:11 Inactive
Copy link
Contributor

@jm90m jm90m left a comment

Choose a reason for hiding this comment

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

I wish there was a way to test this, but going over the code it looks fine. I noticed you used cheerio and handlerbars.js, I guess thats fine since it makes it easier to create the AMP pages. https://github.com/cheeriojs/cheerio... has 14k stars and last update 5 months ago, so not too bad to use, but i just hate jquery :)... anyways lgtm, im not as thorough with backend stuff as I am with frontend but the basic template you have in amp_index.hbs looks fine for me, just kinda wish we could test it somehow. oh well if it breaks, we can always fix it :)

@bonustrack
Copy link
Contributor

@bonustrack
Copy link
Contributor

image

@Sekhmet
Copy link
Contributor Author

Sekhmet commented Dec 17, 2017

I've clicked on the link you provided and I can see preview:
image
And that's what I see for: https://busy-master-pr-1205.herokuapp.com/utopian-io/@sekhmet/my-busy-org-october-report-voting-slider-avatar-lightbox-and-a-lot-more/amp
image

@bonustrack
Copy link
Contributor

@jm90m is the image preview visible for you?

@Sekhmet
Copy link
Contributor Author

Sekhmet commented Dec 17, 2017

@jm90m
About handlebars:
Currently we use normal HTML pages and HTML comments for inserting content on server. It's fine for SSR, but not very effective for AMP template (which needs a lot more data). With handlebars we can manipulate AMP page without even touching JS code, because handlebars handles logic on template level. You can test it the same way as HTML templates (and it both would be pretty boring, because both .hbs and .html would not have dynamic content.

About cheerio:
we need virtual DOM manipulator on server because we need something to parse post contents. We have really three options there (we can't use native DOM manipulation as we don't have access to it on the server):

  1. Parsing it manually (Regex or parsing it in other way).
  2. JSDOM - which is strict about HTML correctness (not a good thing for dealing with HTML content coming from a lot of users and a lot of different clients), slower and heavier.
  3. cheerio - which is faster and handles not-so-correct HTML pages better. It only uses jQuery syntax (which I think is still great).
    I picked cheerio because I was already familiar with it and it seems to fit our needs well.

@jm90m
Copy link
Contributor

jm90m commented Dec 17, 2017

@bonustrack Heres what I see from your link:
screen shot 2017-12-17 at 5 51 33 pm
screen shot 2017-12-17 at 5 51 38 pm
screen shot 2017-12-17 at 5 51 42 pm

@jm90m
Copy link
Contributor

jm90m commented Dec 17, 2017

@Sekhmet thanks for the in-depth explanations, makes sense, looks like cheerio was a good choice 👍

Copy link
Contributor

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

Ho! Ok the preview is working actually i just had to switch tab for "Type of result 2".

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.

3 participants