Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix Bug 965823 - Add a custom Twitter timeline widget to the Student Ambassadors landing page #1642

Merged
merged 1 commit into from Apr 7, 2014

Conversation

Projects
None yet
5 participants
Member

kyoshino commented Jan 30, 2014

How to test:

  1. Install and run memcached
  2. Add the Memcached config and Twitter config in local.py
  3. Run ./manage.py cron update_tweets followed by ./manage.py runserver

@alexgibson alexgibson and 1 other commented on an outdated diff Jan 30, 2014

bedrock/base/templates/macros.html
@@ -280,3 +280,66 @@
{% macro twitter_share_url(url, tweet_text) -%}
{{ 'https://www.twitter.com/intent/tweet?url=%s&text=%s'|format(url|urlencode, tweet_text|urlencode)|e }}
{%- endmacro %}
+
+{% macro twitter_follow_button(name, screen_name, show_screen_name=True) -%}
+ {%- if show_screen_name -%}
+ {% set label = _('Follow @%s')|format(screen_name) %}
+ {%- else -%}
+ {% set label = _('Follow') %}
+ {%- endif -%}
+ <a href="https://twitter.com/{{ screen_name }}" title="{{ _('Follow %s on Twitter')|format(name) }}" class="twitter-follow-button">{{ label }}</a>
+{%- endmacro %}
+
+{% macro twitter_timeline_widget(name, screen_name, tweets, heading_level=3) -%}
+ {% set heading_level = heading_level|int(3) %}
+ <article class="twitter-timeline-widget" itemscope itemtype="http://schema.org/Blog">
@alexgibson

alexgibson Jan 30, 2014

Member

Just curious, is http://schema.org/Blog an accurate representation for the meta-data of a Twitter timeline?

@kyoshino

kyoshino Jan 30, 2014

Member

Hope so since it's a microblog! http://schema.org/Article cannot be nested, fyi.

@pmclanahan pmclanahan and 1 other commented on an outdated diff Jan 30, 2014

bedrock/mozorg/cron.py
@@ -15,3 +17,14 @@ def update_feeds():
# Cache for a year (it will be set by the cron job no matter
# what on a set interval)
cache.set('feeds-%s' % name, feed_info, 60 * 60 * 24 * 365)
+
+
+@cronjobs.register
+def update_tweets():
+ for account in settings.TWEETS:
+ try:
+ tweets = TwitterAPI(account).user_timeline(screen_name=account)
+ except:
+ tweets = []
+
+ cache.set('tweets-%s' % account, tweets, 3600) # Cache for 1 hour
@pmclanahan

pmclanahan Jan 30, 2014

Owner

Not sure the cache is the best place for this since it's so ephemeral. But if we do want to use cache, then set the timeout to much longer so it could potentially withstand an error or two from twitter.

@kyoshino

kyoshino Jan 30, 2014

Member

Okay, will set the timeout 1 year like update_feeds().

@pmclanahan pmclanahan and 1 other commented on an outdated diff Feb 3, 2014

bedrock/mozorg/views.py
@@ -197,6 +199,25 @@ def plugincheck(request, template='mozorg/plugincheck.html'):
return l10n_utils.render(request, template, data)
+def contribute_studentambassadors_landing(request):
+ account = 'mozstudents'
+ cache_key = 'tweets-%s' % account
+ tweets = cache.get(cache_key)
+
+ # Tweets are retrieved by a cronjob. This is a fallback:
+ if tweets is None:
@pmclanahan

pmclanahan Feb 3, 2014

Owner

I think we should not do this as part of the request. If the cache is empty, we should just not show the widget. This opens up the possibility of a thundering-herd problem where we hit twitter potentially hundreds of times in a short period.

@kyoshino

kyoshino Feb 3, 2014

Member

Removed the fallback code.

Member

kyoshino commented Feb 6, 2014

Here's a config in my local.py containing Twitter key and token for test.

Member

kyoshino commented Feb 11, 2014

Filed Bug 970771 to obtain an API key for production.

Member

alexgibson commented Feb 11, 2014

I've tried running this locally with the key/token for testing but I'm running into the following error when visiting the student ambassadors landing page on line 94:

object of type 'NoneType' has no len()

Is there a trick to getting the cron job to run locally, or a step that I'm missing?

Member

kyoshino commented Feb 11, 2014

Oops, fixed the NoneType error.

./manage.py cron update_tweets runs the cron job as documented but the cached tweets cannot be retrieved in the view. Memcached is really short-lived?

Member

kyoshino commented Feb 12, 2014

Ouch, I've forgot to install Memcached; was using the default LocMemCache.

  1. Install and run Memcached
  2. Add the Memcached config and Twitter config in local.py
  3. Run ./manage.py cron update_tweets followed by ./manage.py runserver

It works!

Member

alexgibson commented Feb 12, 2014

I've installed Memcached and followed the instructions here, but I still don't see any tweets sigh.

In the console I'm seeing:

[12/Feb/2014 01:46:25] code 400, message Bad HTTP/0.9 request type ('get')
[12/Feb/2014 01:46:25] "get :1:tweets-mozstudents" 400 -

And also:

[12/Feb/2014 01:46:25] code 400, message Bad HTTP/0.9 request type ('get')
[12/Feb/2014 01:46:25] "get :1:translations:tabzilla/tabzilla" 400 -

@alexgibson alexgibson and 1 other commented on an outdated diff Feb 12, 2014

media/css/libs/twitter.less
@@ -0,0 +1,178 @@
+@import "../sandstone/lib.less";
+@import "elements.less";
@alexgibson

alexgibson Feb 12, 2014

Member

I believe elements.less is a dependency for the Mozilla SocialShare widget. It's built to be used outside of bedrock, on any website - which is why it has its own set if mixins. Any specific reason why you've chosen to use that here, instead of the ones provided in sandstone?

@kyoshino

kyoshino Feb 13, 2014

Member

Looks like I just wanted to use the .gradient and .inner-shadow mixins. Will use one in sandstone/lib.less and remove the dependency on elements.less.

@alexgibson alexgibson and 1 other commented on an outdated diff Feb 12, 2014

media/css/libs/twitter.less
+
+@font-face {
+ font-family: 'Font Awesome';
+ src: url('/media/fonts/fontawesome-webfont.eot?#iefix') format('embedded-opentype'),
+ url('/media/fonts/fontawesome-webfont.woff') format('woff'),
+ url('/media/fonts/fontawesome-webfont.ttf') format('truetype');
+ font-weight: normal;
+ font-style: normal;
+}
+
+.twitter-follow-button {
+ .inline-block;
+ border: 1px solid #CCC;
+ border-radius: 3px;
+ padding: 0 5px;
+ color: #333 !important;
@alexgibson

alexgibson Feb 12, 2014

Member

Please remove uses of !important here and below unless there is a reason why it can't be avoided? Perhaps you need to rework your selectors if you find you're needing to use this a lot.

@kyoshino

kyoshino Feb 13, 2014

Member

Will try to remove !important.

@kyoshino

kyoshino Feb 13, 2014

Member

Ahh, I remember why I have used !important many times: the timeline is embedded directly in the page. Try to use an <iframe> like the official Twitter widget would be a solution here.

@alexgibson alexgibson and 1 other commented on an outdated diff Feb 12, 2014

media/css/libs/twitter.less
+
+ header {
+ .timestamp {
+ float: right;
+ margin: 0 0 0 10px;
+ font-size: @smallFontSize;
+ line-height: @baseLine;
+ letter-spacing: 0;
+
+ a {
+ color: @textColorTertiary;
+ }
+
+ .full {
+ position: absolute;
+ left: -99999px;
@alexgibson

alexgibson Feb 12, 2014

Member

There's a .visually-hidden() helper in sandstone that might help you here. -99999px might be a little heavy handed :)

Member

alexgibson commented Feb 12, 2014

Ah, success I have the timeline working - I just hadn't started the Memcached daemon running.

Member

alexgibson commented Feb 12, 2014

Not a blocker, but at first I didn't realize there we're more tweets to read here and this was an overflow area with scrolling. It was only because of the cropped image in the third tweet that I thought to try?

timeline

Just a suggestion, but maybe we could make use of the empty column to the right and display a set number of tweets (say 3 or 4 in each column). Then for smaller view ports we could switch to a single column? It may not be as straight forward given that you're trying to keep this an a macro which we can embed, but for this page it might make use of the space a little better.

I'll leave it up to you to decide :)

@alexgibson alexgibson and 1 other commented on an outdated diff Feb 12, 2014

bedrock/base/templates/macros.html
+
+{% macro twitter_timeline_widget(name, screen_name, tweets, heading_level=3) -%}
+ {% set heading_level = heading_level|int(3) %}
+ <article class="twitter-timeline-widget" itemscope itemtype="http://schema.org/Blog">
+ <header itemprop="name">
+ <h{{ heading_level }}>{{ _('Tweets from %s')|format(name) }}</h{{ heading_level }}>
+ </header>
+ {% for _tweet in tweets %}
+ {%- if _tweet.retweeted_status -%}
+ {% set retweet = true %}
+ {% set tweet = _tweet.retweeted_status %}
+ {%- else -%}
+ {% set retweet = false %}
+ {% set tweet = _tweet %}
+ {%- endif -%}
+ <article itemprop="blogPost" itemscope itemtype="http://schema.org/BlogPosting">
@alexgibson

alexgibson Feb 12, 2014

Member

I wonder if it would be more appropriate for each tweet to be a list item rather than an <article>? I understand the association with a blog post in terms of meta data, but the parent element is already and <article>, and these are a list of tweets, so semantically perhaps an <li> would be preferred here. Thoughts?

@kyoshino

kyoshino Feb 13, 2014

Member

That's true. I'll include each tweet in <li>.

@kyoshino

kyoshino Feb 13, 2014

Member

Hmm, I have checked some <article> examples. Looks like nested <article>s are okay, and we can also have <header>s and <header>s. The problem here would be the outer <article>. It should be just an <section> since it's not actually an article. Each tweet is an article.

Member

kyoshino commented Feb 13, 2014

Right, on the recent 🍎 OS X and mobile, the scrollbar is hidden by default, it may be difficult to know there are more tweets. Maybe the latest 5 tweets without scroll are enough here. So the left column will include Who We Are, Benefits of Joining, What We Do, Get in Touch, and the right column include the timeline with 5 tweets.

Member

kyoshino commented Feb 13, 2014

Moved the timeline to an <iframe> and addressed CSS issues @alexgibson pointed out. Also adjusted the layout to show the 5 latest tweets at once. (screenshot) 🐱

@icaaq icaaq and 1 other commented on an outdated diff Feb 13, 2014

bedrock/base/templates/macros.html
@@ -280,3 +280,16 @@
{% macro twitter_share_url(url, tweet_text) -%}
{{ 'https://www.twitter.com/intent/tweet?url=%s&text=%s'|format(url|urlencode, tweet_text|urlencode)|e }}
{%- endmacro %}
+
+{% macro twitter_follow_button(name, screen_name, show_screen_name=True) -%}
+ {%- if show_screen_name -%}
+ {% set label = _('Follow @%s')|format(screen_name) %}
+ {%- else -%}
+ {% set label = _('Follow') %}
+ {%- endif -%}
+ <a href="https://twitter.com/{{ screen_name }}" title="{{ _('Follow %s on Twitter')|format(name) }}" class="twitter-follow-button">{{ label }}</a>
+{%- endmacro %}
+
+{% macro twitter_timeline_widget(view_name) -%}
+ <iframe src="{{ url(view_name) }}" class="twitter-timeline-widget"></iframe>
@icaaq

icaaq Feb 13, 2014

Contributor

Label this iframe with a title-attribute that describes the content please :)
http://www.w3.org/TR/WCAG20-TECHS/H64

@kyoshino

kyoshino Feb 13, 2014

Member

Added the title :)

@alexgibson alexgibson commented on an outdated diff Feb 13, 2014

...tes/mozorg/contribute/studentambassadors/landing.html
</div>
</div>
{% endblock %}
{% block email_form %}{% endblock %}
+
+{% block js %}
+ <script>
@alexgibson

alexgibson Feb 13, 2014

Member

Please move this JS to an external file (I know it's small, but it's something we try to avoid).

Member

alexgibson commented Feb 13, 2014

Since moving the timeline to an iFrame, clicking or interacting with a link or image breaks the page :(

Was this just to get around the CSS issues? If you can't remove the uses of !important and use more specific selectors styling, maybe it is better to use that rather than an iFrame solution that also has a JavaScript dependency?

Member

alexgibson commented Feb 13, 2014

New layout looks great! 👍

Member

kyoshino commented Feb 13, 2014

Since moving the timeline to an iFrame, clicking or interacting with a link or image breaks the page :(

Ahhhh, <a> needs target="_blank" or target="_top"to open the link outside the <iframe>, or it breaks due to X-Frame-Options of Twitter.

Actually that CSS will be loaded everywhere Twitter timeline is embedded, so there are many chances of style overrides and !important are needed. An <iframe> can avoid such an issue. But... hmm...

Member

alexgibson commented Feb 13, 2014

I see, in terms of future proofing should this be used on more pages I guess an iframe might then be the easiest option?

Member

kyoshino commented Feb 13, 2014

Yeah, an iframe is the easiest solution and <base target="_blank"> can avoid the X-Frame-Options issue, but... maybe an in-page timeline would be better for a seamless experience. (I just notice that the text color in the iframe is different from other part of the page.) Sorry for the back and forth. Will remove the iframe.

Member

kyoshino commented Feb 13, 2014

If the timeline is in that page, GA event tracking, if needed, can be done easier.

Member

kyoshino commented Feb 13, 2014

Removed the iframe. The usage of !important would be acceptable here. Also adjusted the styles for RTL languages.

Member

alexgibson commented Feb 14, 2014

Not sure if it's just my setup, but I can no longer see the timeline since the last update?

Member

kyoshino commented Feb 14, 2014

Somehow the access token was reset when I visited the Twitter developer page. I have regenerated a new access token and secret so please use this one.

Member

alexgibson commented Feb 17, 2014

Looking good thanks @kyoshino - this gets an r+ from me on the front-end.

Maybe @pmclanahan or @jgmize can take a look at the Python bits?

Member

kyoshino commented Feb 19, 2014

We've got an API key for production. I'm adding GA even tracking.

Member

kyoshino commented Feb 28, 2014

Implemented GA event tracking. Added the Tweepy library to the submodule. All set.

@pmclanahan pmclanahan and 1 other commented on an outdated diff Feb 28, 2014

bedrock/settings/base.py
@@ -821,6 +825,11 @@ def JINJA_CONFIG():
'mozilla': 'https://blog.mozilla.org/feed/'
}
+# Twitter accounts to retrieve tweets with the API
+TWEETS = (
@pmclanahan

pmclanahan Feb 28, 2014

Owner

shouldn't this be TWITTER_ACCOUNTS then, as "tweets" usually refer to the actual posts.

Member

alexgibson commented Mar 3, 2014

We discussed this in our PR triage and we'd like to suggest including a generic fallback message should the timeline fail to display. This message could include a link to the twitter account, so the column won't look quite so empty and the user can still view tweets should something go wrong with the cache.

Member

kyoshino commented Mar 3, 2014

Because there's already a link to the Twitter account, a fallback message and another link might be redundant. The page could just fallback to the current 2-column layout without timeline when tweets are not available. Will update the PR.

Member

alexgibson commented Mar 5, 2014

@kyoshino - from our previous discussion we'd like to get this up on a demo server for more testing.

Do you know if a bug has been filed with IT to use our new prod API key in our various bedrock environments?

Member

kyoshino commented Mar 6, 2014

👍 for testing this on a demo server with a prod key. I haven't filed an IT bug yet though.

Member

kyoshino commented Mar 13, 2014

Filed a WebOps bug to deploy the API key.

Member

kyoshino commented Mar 14, 2014

The API key has been deployed to the demo and dev servers. Is there an available demo server?

Member

alexgibson commented Mar 18, 2014

I've pushed this branch to demo3:

https://www-demo3.allizom.org/en-US/contribute/studentambassadors/

No twitter timeline :(

Any ideas @pmclanahan or @jgmize?

Owner

pmclanahan commented Mar 18, 2014

@alexgibson yeah. We've gotta run a command. We'll have to ask IT to do it via cron in future, but I can manually do it for now.

Member

kyoshino commented Mar 19, 2014

Kate prefers the original layout, so I have put the timeline underneath, in 2 columns using CSS multi-column layout, instead of the right side.

Member

alexgibson commented Mar 19, 2014

Kate prefers the original layout, so I have put the timeline underneath, in 2 columns using CSS multi-column layout, instead of the right side.

I've pushed the latest changes to demo3 again. CSS multi-column layout is only supported in very recent versions of some browsers (http://caniuse.com/#feat=multicolumn). Are we ok with non-supporting browsers displaying tweets at full width for this design?

@alexgibson alexgibson commented on an outdated diff Mar 19, 2014

media/css/base/social-widgets.less
+}
+
+// Twitter timeline widget
+
+section.twitter-timeline-widget {
+ header {
+ a.twitter-follow-button {
+ float: right;
+ margin: -32px 0 0;
+ }
+ }
+
+ article {
+ overflow: hidden;
+ padding: 10px 10px 10px 68px;
+ line-height: @baseLine !important;
@alexgibson

alexgibson Mar 19, 2014

Member

I remember you said you had trouble removing all these uses of !important, but I can't help feeling it should really be a last resort only, as over reliance can be a slippery slope.

I understand this is meant to be a widget as such, but we could probably remove all the occurrences of !important if we added more specificity by using an id on the parent. That would limit the widget to a single occurrence per-page however. But what is the probability of using more than one timeline on a single page, I'm not too sure.

I'd be interested in what @craigcook thinks here?

Member

craigcook commented Mar 19, 2014

I think the liklihood of multiple timeline widgets on a page is pretty slim. If it ever comes up we can just tackle the problem of duplicate IDs at that time. Eliminating !important is a good thing to do right now even if it does make this widget a singleton.

If we really had to avoid an ID the next best option would be a chain of classes to get the higher specificity... but something like .twitter-timeline-widget .twitter-timeline-list .twitter-timeline-item .twitter-timeline-tweetcontent feels even wronger than an ID.

Member

kyoshino commented Mar 19, 2014

Hmm, true, 1 timeline on 1 page, so an ID should work.

Member

kyoshino commented Mar 19, 2014

Used an ID to avoid using !important.

CSS multi-column layout is only supported in very recent versions of some browsers
(http://caniuse.com/#feat=multicolumn).

This chart only shows recent versions. Even Firefox 1.5 supports multi-column layout:
https://developer.mozilla.org/en-US/docs/Web/CSS/column-count#Browser_compatibility

Member

alexgibson commented Mar 19, 2014

This chart only shows recent versions. Even Firefox 1.5 supports multi-column layout:
https://developer.mozilla.org/en-US/docs/Web/CSS/column-count#Browser_compatibility

Yes my question on browser compatibility was more related to IE support, rather than evergreen browsers. But like I say, not an issue as long as we are ok with the design degrading this way on those browsers.

Member

kyoshino commented Mar 19, 2014

Yeah, it looks plain on older IE. But I believe most people visiting this page are Firefox user :)

Member

alexgibson commented Mar 20, 2014

r+ from me on the front end, nice work @kyoshino! 🌴

Member

kyoshino commented Mar 25, 2014

The timeline on the demo3 server is outdated. Is the cronjob not working?

Owner

pmclanahan commented Mar 25, 2014

Did we add the cronjob? I think I just ran it manually, which I can do again if needed. Don't think we need to add a cronjob to demo3 until this is in master since it will fail when a different branch is pushed.

Member

kyoshino commented Mar 25, 2014

I'm not sure how cronjobs works... no problem as long as it works on production.

Member

alexgibson commented Mar 31, 2014

Looks like this one needs a rebase

Member

kyoshino commented Mar 31, 2014

Rebased!

Member

kyoshino commented Apr 3, 2014

Should we install and test cronjobs on the demo server before this PR gets merged to production?

Owner

pmclanahan commented Apr 3, 2014

I don't think that's necessary since we've manually run the command, it should run from cron just as well. We've got many things happening via cron now, so I feel like we're pretty confident in that method. We'll just have to run it manually once before the cron job is added.

Member

kyoshino commented Apr 3, 2014

Okay, that makes sense!

Member

craigcook commented Apr 7, 2014

tip

Member

kyoshino commented Apr 7, 2014

🐹

Member

alexgibson commented Apr 7, 2014

r+ sorry this one took so long @kyoshino 🍻

@alexgibson alexgibson added a commit that referenced this pull request Apr 7, 2014

@alexgibson alexgibson Merge pull request #1642 from kyoshino/bug-965823-ambassadors-twitter
Fix Bug 965823 - Add a custom Twitter timeline widget to the Student Ambassadors landing page
77aef45

@alexgibson alexgibson merged commit 77aef45 into mozilla:master Apr 7, 2014

1 check passed

default Jenkins build 'bedrock_github' #3743 has succeeded
Details

@kyoshino kyoshino deleted the kyoshino:bug-965823-ambassadors-twitter branch Apr 7, 2014

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