Closed
Bug 475584
Opened 15 years ago
Closed 15 years ago
Link Sharing
Categories
(addons.mozilla.org Graveyard :: Public Pages, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
5.0.3
People
(Reporter: osunick, Assigned: lorchard)
References
()
Details
Attachments
(6 files, 5 obsolete files)
Neil, Can we get a design on http://docs.google.com/Doc?id=dds6vwb4_12cnx3dbf6 that we can hand off to engineering? Ping me with questions. Thanks! -n.
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → neilio
Comment 1•15 years ago
|
||
Here's a first run at this. The idea is this would popup when the "Share this" button gets mouseover. The tiny boxes in the popup are the service favicons. When you click on the service link the service page loads with that add-on's information added, etc. We could open the service page in a floating div or in a pop-up window but I tend to prefer just sending them to the page, back button ftw and all of that.
Comment 2•15 years ago
|
||
Based on input from osunick and others, here's another run at this with a completely different direction.
Comment 3•15 years ago
|
||
Reporter | ||
Comment 4•15 years ago
|
||
Love the last wireframe... looks great.
Comment 5•15 years ago
|
||
A couple of notes on how this should work (probably obvious): - the Share This flyout should appear on click - an animation (fade in or scroll out) would be great, but not essential - the flyout should close when users click on the close box, select a service, or click anywhere outside of the flyout itself. - selecting a service should open its page in the current window with the exception of Facebook (which does not send the user back to the referring page).
Comment 6•15 years ago
|
||
Are you really sure we only want those 5?!
Reporter | ||
Comment 7•15 years ago
|
||
Um, yeah. We can add more as people request them, to avoid the situation you just outlined.
Reporter | ||
Comment 8•15 years ago
|
||
Actually, we should add Reddit, so one more. I updated the spec with a link to the button code.
Comment 9•15 years ago
|
||
Thanks Nick. Just to be clear, we're going with attachment 360578 [details] and it should show on add-on detail pages only?
Assignee: neilio → lorchard
Target Milestone: 5.0.2 → 5.0.3
Reporter | ||
Comment 10•15 years ago
|
||
Correct!
Assignee | ||
Comment 11•15 years ago
|
||
This patch implements link sharing. The share count clicks are tracked in the remora DB, since the APIs to query counts for each of the link sharing services are inconsistent or non-existent except for one or two of them.
Attachment #365668 -
Flags: review?(rdoherty)
Assignee | ||
Comment 12•15 years ago
|
||
Attaching a tarball of images that would otherwise be nasty in a patch
Assignee | ||
Comment 13•15 years ago
|
||
Just after uploading the patch, noticed that the addon box was too short when the description was small. so, one more tweak
Attachment #365668 -
Attachment is obsolete: true
Attachment #365672 -
Flags: review?(rdoherty)
Attachment #365668 -
Flags: review?(rdoherty)
Assignee | ||
Updated•15 years ago
|
Attachment #365672 -
Attachment is patch: true
Attachment #365672 -
Attachment mime type: application/octet-stream → text/plain
Comment 14•15 years ago
|
||
Comment on attachment 365672 [details] [diff] [review] added a min-height for the addon box This looks pretty good. I'm only a little worried about 3 things: The font size for 'Share This' is pretty tiny, not sure how well it will work for Arabic, Japanese, etc. If the text for 'Share This' gets any longer, it will expand into the description area. I'm worried about catching share counts because the Netscalers will cache everything that's a GET request. We could add max-age:0 to the headers to prevent this.
Attachment #365672 -
Flags: review?(rdoherty) → review+
Comment 15•15 years ago
|
||
Trying to do real time stat tracking has not gone well for us in the past which is why we do all our stats from the logs now. Since this uses external services we may not have a choice but this is going to need load testing before it can go live. At first glance stuff like "SELECT `service`, SUM(`count`)" worries me. Also, the netscalers haven't respected all the headers we set. I'm not sure how Zeus feels about it but that is something else that will need plenty of testing.
Assignee | ||
Comment 16•15 years ago
|
||
(In reply to comment #14) > (From update of attachment 365672 [details] [diff] [review]) > The font size for 'Share This' is pretty tiny, not sure how well it will work > for Arabic, Japanese, etc. > > If the text for 'Share This' gets any longer, it will expand into the > description area. I made some tweaks to widen the button a bit, so there's some expansion room to fit as many as 4-5 average English words. Hopefully that will work without too much compromise. > I'm worried about catching share counts because the Netscalers will cache > everything that's a GET request. We could add max-age:0 to the headers to > prevent this. To address this, I threw a &__={UNIQ} param on the end, which is updated on the client in javascript with the current time in milliseconds. This should make each of the link sharing links look new to the cache when clicked. (In reply to comment #15) > Trying to do real time stat tracking has not gone well for us in the past which > is why we do all our stats from the logs now. Since this uses external > services we may not have a choice but this is going to need load testing before > it can go live. This doesn't need to be real-time per se, and should be cached by the netscaler. Since I'll need to research how we do load testing for AMO, this won't make 5.0.3 code freeze. This could conceivably be done by logs, though, since the sharing URLs take the form of: http://dev.addons.mozilla.org/en-US/firefox/addon/{ADDON ID}/share?service={SERVICE NAME}&__=12341234234 I don't know what the next steps for counting by logs would be, though. We could hold off on this feature until that's worked out, if it's a big enough issue. > At first glance stuff like "SELECT `service`, SUM(`count`)" worries me. Counters by service are maintained by date, so worst case if an addon got shared every day for a year, there'd be 5 * 365 = 1825 rows to sum, which mysql should handle pretty well. If it's an issue, this can be cached in memcache, and should already get cached by netscaler > Also, the netscalers haven't respected all the headers we set. I'm not sure > how Zeus feels about it but that is something else that will need plenty of > testing. Not going to use headers, using instead a parameter based on client-side time to keep the link URLs changing
Reporter | ||
Comment 17•15 years ago
|
||
How about we separate out the counters into phase 2 and target 5.0.4 (assuming Les has time with his other commitments)?
Comment 18•15 years ago
|
||
> > I'm worried about catching share counts because the Netscalers will cache > > everything that's a GET request. We could add max-age:0 to the headers to > > prevent this. > > To address this, I threw a &__={UNIQ} param on the end, which is updated on the > client in javascript with the current time in milliseconds. This should make > each of the link sharing links look new to the cache when clicked. Can that be some kind of hash instead? A thousand hits in a second isn't inconceivable on an add-on's page (on a rating button, it's stretching it, but that's what we do!) > (In reply to comment #15) > > Trying to do real time stat tracking has not gone well for us in the past which > > is why we do all our stats from the logs now. Since this uses external > > services we may not have a choice but this is going to need load testing before > > it can go live. > > This doesn't need to be real-time per se, and should be cached by the > netscaler. Since I'll need to research how we do load testing for AMO, this > won't make 5.0.3 code freeze. > > This could conceivably be done by logs, though, since the sharing URLs take the > form of: > > http://dev.addons.mozilla.org/en-US/firefox/addon/{ADDON > ID}/share?service={SERVICE NAME}&__=12341234234 > > I don't know what the next steps for counting by logs would be, though. We > could hold off on this feature until that's worked out, if it's a big enough > issue. I assumed it would need to be real time because we were keeping a counter next to the button and people would expect it to go up. If that's not a big deal I'm definitely for log counting. > > At first glance stuff like "SELECT `service`, SUM(`count`)" worries me. > > Counters by service are maintained by date, so worst case if an addon got > shared every day for a year, there'd be 5 * 365 = 1825 rows to sum, which mysql > should handle pretty well. If it's an issue, this can be cached in memcache, > and should already get cached by netscaler We've already got several caching tables for our stats, let's use those again (they are also by date). They are calculated offline by cron jobs.
Assignee | ||
Comment 19•15 years ago
|
||
Okay, made tweaks to widen the button, and excised the database link share counters.
Attachment #365672 -
Attachment is obsolete: true
Assignee | ||
Comment 20•15 years ago
|
||
Comment on attachment 366430 [details] [diff] [review] link share counts removed; CSS tweaked for wider button Adding some r?'s for good measure, but I think this should do it.
Attachment #366430 -
Flags: review?(rdoherty)
Attachment #366430 -
Flags: review?(clouserw)
Assignee | ||
Comment 21•15 years ago
|
||
Ugh, another patch.
Attachment #366430 -
Attachment is obsolete: true
Attachment #366442 -
Flags: review?(rdoherty)
Attachment #366430 -
Flags: review?(rdoherty)
Attachment #366430 -
Flags: review?(clouserw)
Assignee | ||
Comment 22•15 years ago
|
||
Attachment #366442 -
Attachment is obsolete: true
Attachment #366445 -
Flags: review?(rdoherty)
Attachment #366442 -
Flags: review?(rdoherty)
Assignee | ||
Comment 23•15 years ago
|
||
Attachment #366445 -
Attachment is obsolete: true
Attachment #366448 -
Flags: review?(rdoherty)
Attachment #366445 -
Flags: review?(rdoherty)
Comment 24•15 years ago
|
||
Comment on attachment 366448 [details] [diff] [review] or maybe this time, without the count placeholders in constants.php Looks really good. Yay for good JS! Note: I do see %s placeholders, but I'm pretty sure it's an issue with my local install.
Attachment #366448 -
Flags: review?(rdoherty) → review+
Comment 25•15 years ago
|
||
Thanks guys. Can you put the table you had back in to remora.sql when you commit? Let's get it all ready for tracking stats.
Assignee | ||
Comment 26•15 years ago
|
||
checked in as r23044
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•15 years ago
|
||
also checked in as r23045, including the images and SQL this time
Comment 28•15 years ago
|
||
Stats bug: bug 482389 Maintenance bug: bug 482396
Les: would you mind providing a sample URL where this can be verified? Thanks!
Reporter | ||
Comment 30•15 years ago
|
||
check it: https://preview.addons.mozilla.org/en-US/firefox/addon/1843 there's a "share this" button under the screenshot on the left side of the screen.
(In reply to comment #30) > check it: > > https://preview.addons.mozilla.org/en-US/firefox/addon/1843 > > there's a "share this" button under the screenshot on the left side of the > screen. Yeah, sorry for being a dunderhead and not checking preview like a good QA engineer; anyway, verified fixed on the various services. 1) There is some tweaking to be made in 5.0.4, though, like removing the onMouseover event listener and replacing it with onClick (I think!) 2) Also note that Digg doesn't retain the passed-in URL unless you're signed in, but I don't think there's anything we can do about that?
Updated•8 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•