487311 - AMO should support Twitter in its "Share this" menu

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect, P2)

Description

[Rob Campbell [:rc] (:robcee)]

The Share This button doesn't support twitter.com. It should!

Comment 1

[Justin Scott [:fligtar]]

I agree that this is a good idea, with the provision that user 2077082 does not have the Twitter option.

Comment 2

[Rob Campbell [:rc] (:robcee)]

that's me, isn't it?

Comment 4

[Fred Wenzel [:wenzel]]

bug 491775 comment 0 has an example of the HTTP GET interface. Also, do we need to shorten the URL before sending it there, or do we let twitter handle this?

Comment 5

[u278084]

Depending on what is tweeted, we might hit the 140 character limit. The URL itself could be up to 53 characters long.

Comment 6

[Fred Wenzel [:wenzel]]

I agree. Unless I am mistaken, the whole "share this" menu is JS-based, so we might just want to hit up tinyurl (or whatever we prefer) with an AJAX request before sending it off to twitter. (Which leads me to a side question, does Mozilla have any very short domain that could be used as an in-sourced URL shortening service?)

Comment 7

[u278084]

It might also be worth the extra trouble to keep the shortened url in the db, or at least cached. is.gd has a limit of 5K requests per IP per day. tinyurl doesn't seem to have any limit.

Comment 8

[Fred Wenzel [:wenzel]]

We've found out yesterday that twitter does not actually limit the number of characters you submit. So we may just drop it in there and let the user shorten it if necessary. However, I am not sure how bad a UX that is.

Comment 9

[Nick Nguyen [:osunick]]

The get interface lets the user edit the tweet after the fact.  I don't really
like url shorteners in general- the URL contains valuable information about
what it is and actually having links to AMO in twitter helps stuff like SEO. 
Why not just post the URL and the user can add in any additional information
they want.  Then we also don't have to worry about localization and people
don't just get a ton of "Check out this add-on!" in 50 languages.

Comment 10

[u278084]

Attachment: v1

This isn't done because a sprite has to be modified to include twitter. There is some extra code in the share service to fix an issue I was having; I would get a status tweet with entities in them (ie. Foo - Bar)

Comment 11

[Jeff Balogh (:jbalogh)]

Comment on attachment 386217 [details] [diff] [review]
v1

Looks good to me once the sprite is updated.

>Index: site/app/controllers/addons_controller.php
>===================================================================
>--- site/app/controllers/addons_controller.php	(revision 28339)
>+++ site/app/controllers/addons_controller.php	(working copy)
>@@ -100,6 +100,12 @@
>             'friendfeed' => array(
>                 'label' => ___('addons_share_label_friendfeed', 'Share on FriendFeed'),
>                 'url' => 'http://friendfeed.com/?url={URL}&title={TITLE}'
>+            ),
>+
>+            // See Nick Nguyen

Why?

Comment 12

[Wil Clouser [:clouserw]]

Thanks Jeff.  Ceasar: please wait until after 5.0.7 goes live before landing any code.

Comment 13

[u278084]

Attachment: updated patch for rtl

Patch had to be updated to support rtl languages, and because the sprite position was already taken.

(In reply to comment #11)
> (From update of attachment 386217 [details] [diff] [review])
> >+            // See Nick Nguyen
> 
> Why?

Because using a GET request to pre-fill your status is not mentioned in the Twitter API docs. And as Wenzel pointed out, Nick found this out in bug 491775 comment 0. That makes Nick our special Twitter API Doc :)

Comment 14

[u278084]

Attachment: sprite

To be put in webroot/img/amo2009/icons/

Comment 15

[Jeff Balogh (:jbalogh)]

Comment on attachment 390148 [details] [diff] [review]
updated patch for rtl

Thanks!  Good catch on the RTL.  I would put the offset at -1498px instead of -1500px; that makes the bottom of the twitter logo line up with our text.

Comment 16

[u278084]

Thanks Jeff, offset changed. Code changes in r30403. Locale changes in r30404.

Comment 17

[raymond [:retornam] (inactive)]

verified fixed on  https://preview.addons.mozilla.org/en-US/firefox/addon/2132