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

@craigspaeth => Distribute Instant Articles via FB API #916

Merged
merged 14 commits into from
Feb 28, 2017

Conversation

eessex
Copy link
Contributor

@eessex eessex commented Feb 6, 2017

No description provided.

@kanaabe
Copy link
Contributor

kanaabe commented Feb 9, 2017

My latest commit separates some of the bloat in save.coffee and moves it to a distribute.coffee. I've put Search, Instant Articles, and Sailthru stuff in there.

Also I got the rendering bit to work but there's definitely cleanup potential there.

Let me know if you have any questions on these parts!

@kanaabe
Copy link
Contributor

kanaabe commented Feb 10, 2017

  • Ensure prepForInstant works
  • Complete Model tests
  • Complete distribute.coffee tests
  • Add a few template tests
  • Test a bunch

I'll probably spend at least a part of my 14 hour flight working on the above haha, but just wanted to get a checklist going for visibility.

moment = require 'moment'
particle = require 'particle'

@distributeArticle = (article, cb) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for extracting these common functions out to its own module.

@kanaabe kanaabe changed the title [WIP] Distribute Instant Articles via FB API @craigspaeth => Distribute Instant Articles via FB API Feb 27, 2017
@kanaabe
Copy link
Contributor

kanaabe commented Feb 27, 2017

All the tests are now in. However this still needs a final gloss-over/polish on my end so I'll ping when ready!

@kanaabe
Copy link
Contributor

kanaabe commented Feb 28, 2017

Okie dokes @craigspaeth, this PR is now ready for review.


include mixins

html(lang="en" prefix="op: http://media.facebook.com/op#")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 at first it seemed a little weird that we're rendering jade templates from the API app, but I think it makes total sense for the use case.

moment = require 'moment'
cheerio = require 'cheerio'

module.exports = class Article extends Backbone.Model
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 clever use of an API Backbone model here!

moment = require 'moment'
particle = require 'particle'
cloneDeep = require 'lodash.clonedeep'

Copy link
Contributor

Choose a reason for hiding this comment

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

👌 well done separating out this distribute module.

article = new Article cloneDeep article
return cb() unless article.isEditorial()
article.prepForInstant()
jade.renderFile 'api/apps/articles/components/instant_articles/index.jade',
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@craigspaeth
Copy link
Contributor

👏 great job! The code is very clean and clear. It puts a smile on my face seeing how much this highlights the effectivness of universal JS even beyond the typical "optimizing initial render" use case. Nice use of the same tools (Backbone, Jade, Cheerio, etc.) to solve a server-side/API problem. Very cool 💯

@craigspaeth craigspaeth merged commit 323e349 into artsy:master Feb 28, 2017
@eessex eessex deleted the fb-api branch July 24, 2017 14:27
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