Amphtml: Add user timeline support to AMP-TWITTER

[teleservizi]

On implementation:
1- Currently amp-twitter requires data-tweetid, we will introduce data-timelineid and make ONE of them required (changes required to validator-amp-twieer.protoascii)
2- amp-twitter.js most likely does not need to change as it just passes data to twitter.js in 3p folder
3- Inside twitter.js we would change export function twitter to check whether data.tweetid or data.timelineid exist and depending on which one is there, it would either do what it does now or fork and uses twttr.widgets.createTimeline instead of twttr.widgets.createTweet.
4- update documentation and tests, etc..

On contributing process:

• [ ] If you aren't too familiar with Git/GitHub, see the Getting Started End-to-End Guide for an intro to Git & GitHub, and how to get a copy of the code. You can also refer to the Quick Start Guide for the necessary setup steps with less explanation than the End-to-End guide.
• [ ] Follow the instructions for building AMP.
• [ ] [Create a Git branch](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#create-a-git-branch) for making your changes.
• [ ] Implement the solution.
• [ ] [Commit your changes](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#edit-files-and-commit-them) frequently.
• [ ] [Push your changes to GitHub](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#push-your-changes-to-your-github-fork).
• [ ] [Create a Pull Request](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#send-a-pull-request-ie-request-a-code-review). Mention closes Issue #300 in the description.
• [ ] Assign @aghassemi as the reviewer
• [ ] [Respond to your reviewer's comments](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#respond-to-pull-request-comments) (if any).

[adelinamart]

@niallkennedy are you still working on this? Thanks.

[niallkennedy]

I have no current plans to develop this component.

[adelinamart]

@ericlindley-g to decide what to do with this FR.

[joshprice]

Is anyone working on this? This is definitely something I'd love to see in the component library. If someone could give me some guidance I could have a stab at building this.

Just for some more context, currently the pages where I need this are what you call "dirty AMP" ie an AMP page but because I load the twitter JS to create a custom iframe it makes the page invalid AMP.

[joshprice]

Presumably it would take roughly the same form as https://github.com/ampproject/amphtml/tree/master/extensions/amp-twitter but would it be a new component called amp-twitter-timeline or could it be incorporated into the current twitter component?

[jasti]

hey @aghassemi , was talking to @joshprice at the roadshow and he was interesting in implementing it if we can't prioritize it.
Any chance we could make this a GFI and provide Josh with some guidance?
Also, do we need Twitter's help to support this?

[aghassemi]

@joshprice Thanks for volunteering to contribute. Find below some step by step instructions.

• [ ] If you aren't too familiar with Git/GitHub, see the Getting Started End-to-End Guide for an intro to Git & GitHub, and how to get a copy of the code. You can also refer to the Quick Start Guide for the necessary setup steps with less explanation than the End-to-End guide.
• [ ] Follow the instructions for building AMP.
• [ ] [Create a Git branch](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#create-a-git-branch) for making your changes.
• [ ] Implement the solution.
• [ ] [Commit your changes](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#edit-files-and-commit-them) frequently.
• [ ] [Push your changes to GitHub](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#push-your-changes-to-your-github-fork).
• [ ] [Create a Pull Request](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#send-a-pull-request-ie-request-a-code-review). Mention closes Issue #300 in the description.
• [ ] Assign @aghassemi as the reviewer
• [ ] [Respond to your reviewer's comments](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#respond-to-pull-request-comments) (if any).

[jasti]

Thanks Ali! I think Josh was also looking for some guidance on the implementation/ design - if it isn't too much trouble.

[joshprice]

Thanks @jasti @aghassemi, I was only looking for guidance on implementation and design as what you describe is a fairly standard Github workflow.

[aghassemi]

@joshprice Got it, here is what I think we can do:

1- Currently amp-twitter requires data-tweetid, we will introduce data-timelineid and make ONE of them required (changes required to validator-amp-twieer.protoascii)
2- amp-twitter.js most likely does not need to change as it just passes data to twitter.js in 3p folder
3- Inside twitter.js we would change export function twitter to check whether data.tweetid or data.timelineid exist and depending on which one is there, it would either do what it does now or fork and uses twttr.widgets.createTimeline instead of twttr.widgets.createTweet.
4- update documentation and tests, etc..

Hope this help and thanks again for contributing. Hit me here on on Slack if questions come up.

[nainar]

Hi @joshprice, I was wondering if there had been progress here? If not is it open for others looking for GFI to take on?

[sparhami]

In case anyone is interested in taking up this issue, I recently did a similar change to support Twitter moments in #16167. That should outline most of the code that needs to change.

One thing to note is that if you want to add to the validator-amp-twitter.html, you will need to run gulp validator --update_tests to update the output file correctly.

[felixarntz]

Hi @aghassemi, I'd like to tackle this! It would be my first AMP contribution, so I likely need a bit of time to dive in, but it appears this issue has a clearly defined scope, and with #16167 I have a good reference for what to look at.

I've looked into the Twitter documentation for creating a timeline, and noticed there is something we should likely figure out before writing code:

• The content of a timeline can vary greatly, so specifying what type of timeline to display is a bit more involved than just specifying an ID (like for a tweet or moment), so I'm not sure a simple timelineid is sufficient (unless we make this accept a JSON object).
• Timelines require an object with a sourceType property to specify the type of timeline plus one or two further properties to specify the exact resource to use, based on the specified sourceType.
• I see two ways at the moment that could be implemented with the twitter.js file:
  
  
  
  • Either we require a parameter for the sourceType, for example called timelinesource, and then further arguments, such as screenname, id, etc. That timelinesource would be an addition to the existing tweetid and momentid.
  
  
  • Or we abstract the Twitter API logic a little further by keeping the timeline variants explicitly different content on the API surface, as in we wouldn't expose sourceType in any way, but rather have a couple more new options in addition to the existing tweetid and momentid. Those could be profileid, profilescreenname, likesid, likesscreenname, collectionid, etc.
  
  

Let me know your thoughts, and feel free to assign me.

[aghassemi]

@felixarntz Thanks! That would be amazing.
@sparhami would you please take a look at the proposal and help @felixarntz through his contribution?

[sparhami]

@sparhami would you please take a look at the proposal and help @felixarntz through his contribution?

Sure! More than happy to answer any questions on the code or how to try things out.

Either we require a parameter for the sourceType, for example called timelinesource, and then further arguments, such as screenname, id, etc. That timelinesource would be an addition to the existing tweetid and momentid.

I think having timeline-source (maybe timeline-source-type to more closely match how Twitter names it) would be a good route. That way we know that we need to call createTimeline based on that being present. For the other properties, I think it makes sense to keep the names the same as twitter as long as there isn't a pressing need to make them different.

It may also make sense to simply pass all the converted data- attributes to the createTimeline call so that way if a new option is added, we do not need to be in the loop. Perhaps something like:

twttr.widgets.createTimeline(Object.assign({
  sourceType: data.timelineSourceType,
}, data), tweet, data);

[felixarntz]

@sparhami

It may also make sense to simply pass all the converted data- attributes to the createTimeline call so that way if a new option is added, we do not need to be in the loop.

I like that approach for its flexibility. Something we might wanna do in order to not consider unrelated data attributes is to prefix all data attributes that should be passed to createTimeline with timeline-. We could gather all those attributes present and remove the prefix. This may add a little too much overhead on the other hand, but I wanted to bring it up.

I will work on the implementation later this week.

[sparhami]

I like that approach for its flexibility. Something we might wanna do in order to not consider unrelated data attributes is to prefix all data attributes that should be passed to createTimeline with timeline-. We could gather all those attributes present and remove the prefix. This may add a little too much overhead on the other hand, but I wanted to bring it up.

Sounds good. I don't think we need to worry about the overhead there.

[felixarntz]

@sparhami I have started work on the PR for this. I'm not familiar with how the validator-amp-twitter.html and validator-amp-twitter.out files work and what I would need to adjust there, is there some introductory content on that you can point me to?

[sparhami]

You can find the instructions for installing the stuff necessary / running the validator here: https://github.com/ampproject/amphtml/blob/master/validator/README.md

You can change the html file (https://github.com/ampproject/amphtml/blob/master/extensions/amp-twitter/0.1/test/validator-amp-twitter.html), then run the validator to update the .out file.

You'll want to use --update_tests when running the validator. You can also run it with gulp validator --update_tests.

It does take a few steps to set up and get working, so I'm not sure it is worth going through the effort for this change.

[felixarntz]

Thanks! I will look into this at the end of this week.