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

Twitter display support for Mobile #3342

Open
willdoran opened this Issue Oct 16, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@willdoran
Member

willdoran commented Oct 16, 2018

  • Tweets should be displayed using Twitter api to fetch them.
  • For Posts that contain Tweets or for Conversation with Author sections that are Tweets, the Mobile App should expect to use the Tweet ID to retrieve the Tweet directly from the Twitter API live.
    - this means we should have support for showing a tweet using the data_source_message_id from message.data_source_message_id that will come in the post object
  • Ideally, we should reuse pre-existing elements and not design something completely new.
  • Because we can't save a tweet's content, the description will not be shown.
@rowasc

This comment has been minimized.

Contributor

rowasc commented Oct 18, 2018

@dalezak we have this issue, related to this twitter data import epic #3338 where we need to change the way we display posts that are coming from twitter so that we render the content directly from the Twitter API instead of getting the tweet's content from our database. The Platform API will return the tweet id which should be used to fetch the tweet from Twitter. The post will not have a description or title.

A couple of things

  • I can have the API modifications ready tomorrow EOD.
  • Could you start reviewing and implement a way to show the tweets so once the API is ready we can just test with mobile (assuming that you will get a tweet id in message.data_source_message_id data_source_message_id) ?
  • There's an open question about whether or not we should show the conversation with author section that I will update this ticket with once we resolve it #3334 (comment) I posted the question here

This should be treated as a high priority issue. I am working on the API and will update you tomorrow.

cc @willdoran

@dalezak

This comment has been minimized.

Collaborator

dalezak commented Oct 18, 2018

@willdoran @rowasc @jrtricafort is there a possible simpler solution to this as temporary fix?

I don't know the full requirements, but perhaps a workaround to change the post title to include (from Twitter)?

For example, the post Here's a post hole on my street becomes Here's a post hole on my street (from Twitter)?

@willdoran

This comment has been minimized.

Member

willdoran commented Oct 18, 2018

@dalezak No, unfortunately not. Twitter content can't be stored only the Tweet ID.

@rowasc

This comment has been minimized.

Contributor

rowasc commented Oct 19, 2018

@dalezak sadly no. This is needed because of #3329

@dalezak

This comment has been minimized.

Collaborator

dalezak commented Oct 19, 2018

Hmm @willdoran @rowasc, I'm not sure whether the suggested solution is feasible, or at least not without a lot of effort.

Pulling tweets from Twitter API is a big job, several days of work to wire up the provider, refactor how posts are displayed, etc.

Isn't there another server side workaround? For example rather than displaying the tweet text, display the URL to tweet instead? This way the user can click the URL to show the tweet in the in-browser in the app.

@willdoran

This comment has been minimized.

Member

willdoran commented Oct 19, 2018

@dalezak Why is it difficult to retrieve a tweet with a given ID and display it formatted as a Tweet in Ionic?

This is pretty high priority so if the only option we have is to display the tweet as a link we will have to do that.

@dalezak

This comment has been minimized.

Collaborator

dalezak commented Oct 19, 2018

@willdoran it will just take some time to wire up that functionality:

  • setup a Twitter API key
  • add Twitter NPM module
  • add Twitter provider
  • refactor data model to store data_source_message_id
  • update page to use Twitter provider
  • display that tweet in the page

Perhaps a better option is to just use the Twitter embed?

https://developer.twitter.com/en/docs/twitter-for-websites/embedded-tweets/overview.html

@rowasc

This comment has been minimized.

Contributor

rowasc commented Oct 19, 2018

@dalezak @willdoran I think the embed sounds good enough as a hotfix at least. We can do a follow up issue for better UX after going live next week, I think.

@dalezak

This comment has been minimized.

Collaborator

dalezak commented Oct 19, 2018

@willdoran @rowasc I commented here #3329 (comment), I'm wondering whether displaying Twitter's embed in the app is necessary to comply with Twitter ToS?

For example, if the data was purged or cleaned server side for deleted Tweets, then the mobile app wouldn't require any changes since it would pull those updated posts?

@willdoran

This comment has been minimized.

Member

willdoran commented Oct 19, 2018

@dalezak Can you estimate how long it will take to implement the embed option?

@rowasc

This comment has been minimized.

Contributor

rowasc commented Oct 22, 2018

bumping. @dalezak see @willdoran question above.
Also: note that the data_source_message_id used to load tweets will come in the post directly (edited my original comment)

@rowasc

This comment has been minimized.

Contributor

rowasc commented Oct 30, 2018

@dalezak can we schedule the new twitter display change for this week/the next one?
cc @willdoran

@rowasc rowasc added P1 - Immediate and removed P0 - Urgent labels Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment