AMO should support Twitter in its "Share this" menu

VERIFIED FIXED in 5.0.8

Status

addons.mozilla.org Graveyard
Public Pages
P2
normal
VERIFIED FIXED
9 years ago
2 years ago

People

(Reporter: rc, Assigned: cesar)

Tracking

unspecified
5.0.8

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
The Share This button doesn't support twitter.com. It should!
Summary: AMO should support Twitter in it's "Share This" menu → AMO should support Twitter in its "Share this" menu
I agree that this is a good idea, with the provision that user 2077082 does not have the Twitter option.
(Reporter)

Comment 2

9 years ago
that's me, isn't it?
(Assignee)

Updated

9 years ago
Duplicate of this bug: 491775
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?
(Assignee)

Comment 5

9 years ago
Depending on what is tweeted, we might hit the 140 character limit. The URL itself could be up to 53 characters long.
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?)
(Assignee)

Comment 7

9 years ago
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.
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.
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.

Updated

9 years ago
Priority: -- → P2
Target Milestone: --- → 5.0.7

Updated

9 years ago
Target Milestone: 5.0.7 → 5.0.8
(Assignee)

Comment 10

9 years ago
Created attachment 386217 [details] [diff] [review]
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)
Assignee: nobody → cdolivei.bugzilla
Status: NEW → ASSIGNED
Attachment #386217 - Flags: review?(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?
Attachment #386217 - Flags: review?(jbalogh) → review+
Thanks Jeff.  Ceasar: please wait until after 5.0.7 goes live before landing any code.
Whiteboard: [patch]
(Assignee)

Comment 13

9 years ago
Created attachment 390148 [details] [diff] [review]
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 :)
Attachment #386217 - Attachment is obsolete: true
Attachment #390148 - Flags: review?(jbalogh)
(Assignee)

Comment 14

9 years ago
Created attachment 390149 [details]
sprite

To be put in webroot/img/amo2009/icons/
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.
Attachment #390148 - Flags: review?(jbalogh) → review+
(Assignee)

Comment 16

9 years ago
Thanks Jeff, offset changed. Code changes in r30403. Locale changes in r30404.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: push-needed
Resolution: --- → FIXED
Whiteboard: [patch]
verified fixed on  https://preview.addons.mozilla.org/en-US/firefox/addon/2132
Status: RESOLVED → VERIFIED
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.