Closed
Bug 491581
Opened 15 years ago
Closed 15 years ago
Implement collections promo module
Categories
(addons.mozilla.org Graveyard :: Public Pages, defect)
addons.mozilla.org Graveyard
Public Pages
Tracking
(Not tracked)
VERIFIED
FIXED
5.0.6
People
(Reporter: clouserw, Assigned: rdoherty)
References
Details
(Whiteboard: ->rdoherty)
Attachments
(1 file, 2 obsolete files)
13.11 KB,
patch
|
clouserw
:
review+
|
Details | Diff | Splinter Review |
Once clearleft delivers designs we'll need to implement the promo module. Spec is here: http://docs.google.com/Doc?id=dds6vwb4_15cq79x5gc&hl=en
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → nat
Whiteboard: ->rdoherty
Reporter | ||
Comment 1•15 years ago
|
||
Rough CSS is in the header/footer now with bug 490889 - just need to change the layout to amo2009.
Assignee: nat → rdoherty
Assignee | ||
Comment 2•15 years ago
|
||
Does this bug include the required administration tools required for managing the module?
Reporter | ||
Comment 3•15 years ago
|
||
Sounds like there will only be 5 promoted collections at launch, btw.
Reporter | ||
Comment 4•15 years ago
|
||
Les implemented a version of this in his patch for bug 490906 using the FYF collections hoping it could be easily swapped out. If you haven't started on this you can probably build off of it. I'll add comments over there once I review it.
Comment 5•15 years ago
|
||
Looks like I did too much - my patch in bug 490906 has an implementation of this module, using the FYF collections (look for _findTeaserCollections): https://bugzilla.mozilla.org/attachment.cgi?id=378234 But, this doesn't provide for the homepage manager called for in bug 490667. Hopefully, this isn't wasted work and the view data produced in the _findTeaserCollections() method can be replaced with something more appropriate.
Assignee | ||
Comment 6•15 years ago
|
||
First stab at it. Had trouble reconciling how the promotion categories were tracked, I'd like comments on how _findTeaserCollections() could be written better, thanks!
Attachment #379202 -
Flags: review?(clouserw)
Reporter | ||
Comment 7•15 years ago
|
||
Comment on attachment 379202 [details] [diff] [review] v1 The logic seems to work (although one of the categories didn't show me anything) but on first load if there aren't collections for everything I see everything from array errors to SQL errors so it's hard to tell without some more error checking in there.
Attachment #379202 -
Flags: review?(clouserw) → review-
Assignee | ||
Comment 8•15 years ago
|
||
Added some error checking if no collections are listed for a promo.
Attachment #379202 -
Attachment is obsolete: true
Attachment #379785 -
Flags: review?(clouserw)
Reporter | ||
Updated•15 years ago
|
Attachment #379785 -
Flags: review?(clouserw) → review-
Reporter | ||
Comment 9•15 years ago
|
||
Comment on attachment 379785 [details] [diff] [review] v2 1) Your ternary is broken. It would be better to split complicated stuff like that out into normal if statements. (Example #3 from php.net/ternary if you want to see why it's broken) 2) You're pulling too many add-ons for each side of the carousel. I added a $limit to the function as well as fixed the broken $orderBy so you should change your call to: $this->Addon->getAddonsFromCollection($collectionId, 'RAND()', null, 3); 3) This is odd, but the install buttons are display:none in one of the categories. It's sharing the same add-ons as another category which shows them fine (perhaps it's something to do with being experimental?). I'll try to talk stephend into screencasting this since it's hard to explain. Also, there are 3 other categories that share the same add-ons (non-experimental) that show the buttons fine.
Screencast for Wil's issue: http://screencast.com/t/gCXhhSR7GIf
Assignee | ||
Comment 11•15 years ago
|
||
Fixed issues 1 & 2. #3 is due to addon buttons all get an ID based on the addon id. Because the same addons are listed more than once in a page, they get duplicate ids, which confuses the button code. This kind of problem would have occurred in the old design, but apparently we never encountered it. We can either not show the same addon more than once or spin off a new bug to rewrite the button display and JS code.
Attachment #379785 -
Attachment is obsolete: true
Attachment #380201 -
Flags: review?(clouserw)
Reporter | ||
Comment 12•15 years ago
|
||
Comment on attachment 380201 [details] [diff] [review] v3 suckas! Since #3 isn't a new problem and only affects sandbox add-ons I'm fine with this patch. Also, string changes make me sad.
Attachment #380201 -
Attachment is patch: true
Attachment #380201 -
Attachment mime type: application/octet-stream → text/plain
Attachment #380201 -
Flags: review?(clouserw) → review+
Assignee | ||
Comment 13•15 years ago
|
||
r26489
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Keywords: push-needed
Verified FIXED; it's implemented, and we've already been filing and fixing bugs on it elsewhere.
Status: RESOLVED → VERIFIED
removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"
Keywords: push-needed
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
•