508468 - Add contribution amounts to listings on home page, categories, and search results

Status: VERIFIED FIXED

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

Description

[Nick Nguyen [:osunick]]

These amounts should appear in the listing section under the bandwagon promo module on the front page as well as the category landing pages.  They should link to the "Meet the Developer" page for that add-on.  Also, if the amount is $0.00, they should just say "Support this Add-on".

See attached for mock.

Comment 1

[Nick Nguyen [:osunick]]

Attachment: Mockup of contributions text

Comment 2

[Milos Dinic [:Milos]]

Nice one Nick! I`ll leave this for tomorow. If no one takes it, I can make a patches.

Comment 3

[Milos Dinic [:Milos]]

Attachment: patch v1

* Did everything as on Nick`s mockup 
plus: added 1px solid border
plus: corrected css for displaying install button on cat recommended box

Comment 4

[Milos Dinic [:Milos]]

Attachment: patch v2

Comment 5

[Wil Clouser [:clouserw]]

Comment on attachment 394183 [details] [diff] [review]
patch v2


- You're doing an SQL query in the view - definitely out of place there.  Queries go in the model and their results are passed to the view through the controllers.  In this case though, suggested amount should already be a part of the Addon array I think.

- There is some leeway in views but for the most part code layout should be consistent throughout and with PEAR standards, so:

  if (something) {
  } else {
  }

Some of the code at the bottom of your patch doesn't follow that format.  Since it's a view you can use the template syntax (if/endif) if you think it would help readibility in this case.

- I'm not sure why this line is here:
   if ($rowSuggested['suggested_amount'] != 33) {

Comment 6

[Milos Dinic [:Milos]]

(In reply to comment #5)
> (From update of attachment 394183 [details] [diff] [review])
> 
> - You're doing an SQL query in the view - definitely out of place there. 
> Queries go in the model and their results are passed to the view through the
> controllers.  In this case though, suggested amount should already be a part of
> the Addon array I think.

I have tried to use that, but with no luck. I`ll try to use existing query to get results I need.

> - There is some leeway in views but for the most part code layout should be
> consistent throughout and with PEAR standards, so:
> 
>   if (something) {
>   } else {
>   }
> 

If by that you mean I need to rewrite the code to be pleasant for developers eyes, I`ll do it.

> Some of the code at the bottom of your patch doesn't follow that format.  Since
> it's a view you can use the template syntax (if/endif) if you think it would
> help readibility in this case.
> 
> - I'm not sure why this line is here:
>    if ($rowSuggested['suggested_amount'] != 33) {

I used $33 suggested amount on my local remora for testing, and just forgot to change that to 0. I`ll fix that.

Comment 7

[Milos Dinic [:Milos]]

Attachment: patch v3, edited by Wil

Comment 8

[Wil Clouser [:clouserw]]

Comment on attachment 395090 [details] [diff] [review]
patch v3, edited by Wil

Hey, this patch is getting close.

1) The mockup shows this as a lot more subtle (no border) and I think that's important
2) The category page is completely messed up, CSS wise.  I'll add a screenshot
3) You're re-committing old style L10n.  Instead of ___('placeholder','text') it should just be ___('text') now.

Comment 9

[Wil Clouser [:clouserw]]

Attachment: message with border

Comment 10

[Wil Clouser [:clouserw]]

Attachment: messed up category page

Comment 11

[Milos Dinic [:Milos]]

Attachment: patch v4

Comment 12

[Wil Clouser [:clouserw]]

Comment on attachment 395399 [details] [diff] [review]
patch v4

>+  <?php
>+      if (isset($addon['Addon']['suggested_amount'])):
>+          if ($addon['Addon']['suggested_amount'] != 0) {
>+              $_title = sprintf(___('Contribute $%1$s'), $addon['Addon']['suggested_amount']);
>+          } else {
>+              $_title = ___('Support this add-on');
>+          }
I didn't notice when we were talking on IRC but the mockups always say "Support this add-on".  So, this should be "Support this add-on: Contribute $%1$s" in the first branch of the if().

>+  ?>
>+  
>+  <div class="home-list-contrib-bl">
>+  <?=$html->link($_title, "/addon/{$addonID}/developers", array('class' => 'developersupport'))?>
>+  </div>
>+  
>+  <?php endif; //['Addon']['suggested_amount'] ?> 
>+</div>

^^ That last </div> is what is breaking the category page.  It should be deleted.

>-        <a title="<?=___('Learn more about this add-on') ?>" href="<?=$html->url("/addon/{$addonId}")?>"><?=$addonName?></a>
>-        <span title="<?=___('View Author\'s Profile') ?>"><?=___('by')?> <?=$html->linkUsersFromModel($addonAuthors, 0);?></span>
>+        <a title="<?=___('addons_title_tooltip') ?>" href="<?=$html->url("/addon/{$addonId}")?>"><?=$addonName?></a>
>+        <span title="<?=___('addons_author_tooltip') ?>"><?=_('addons_home_by')?> <?=$html->linkUsersFromModel($addonAuthors, 0);?></span>
You're removing new L10n and replacing it with the old style.  At this point you'll probably have to swap the strings out manually.

>-    <a title="<?=___('Learn more about this add-on') ?>" href="<?=$html->url("/addon/{$addonId}")?>">
>+    <a title="<?=___('addons_title_tooltip') ?>" href="<?=$html->url("/addon/{$addonId}")?>">
Same thing here

>-        <span class="downloads"><strong><?=$html->number_format($addonWeeklyDownloads, 0)?></strong> <?=___('weekly downloads')?></span>
>+        <span class="downloads"><strong><?=$html->number_format($addonWeeklyDownloads, 0)?></strong> <?=___('addon_downloads_weekly')?></span>
and here

>+      if (isset($addon['Addon']['suggested_amount'])):
>+          if ($addon['Addon']['suggested_amount'] != 0) {
>+              $_title = sprintf(___('Contribute $%1$s'), $addon['Addon']['suggested_amount']);
>+          } else {
>+              $_title = ___('Support this add-on');
>+          }
Same title switch needed as above

Comment 13

[Wil Clouser [:clouserw]]

Oh, it looks like in the top image in the mockup the text is slightly smaller than the surrounding text.  Can you add that too?  Thanks! :)

Comment 14

[Milos Dinic [:Milos]]

Attachment: patch v5

Changed text to "Support this add-on: Contribute $%1$s";
Removed last div that was making troubles(from cat landing page);
Text size is now 0.9em (against 1em in <p>'s);
About the l10n strings: I haven`t been updating my files for a while now, and that is why the diff gave so much changes to homepage_addons.thtml

__Hope this'll work__ :)

Comment 15

[Jeff Balogh (:jbalogh)]

Comment on attachment 395442 [details] [diff] [review]
patch v5

It's looking better, good job!

>+.home-list-contrib-bl {
>+    margin-top: 30px;
>+    margin-left: 10px;
>+    text-align: center;
>+    width: 70%;
>+   -moz-border-radius: 8px;

Can you add -webkit-border-radius and plain border-radius?

>+    border: 1px solid transparent;

If the border is transparent, do we actually need it?

>+.developersupport {
>+font-weight: bold;
>+font-size: 0.9em;

Indentation?

>Index: webroot/css/amo2009/main.css
>===================================================================
>--- webroot/css/amo2009/main.css	(revision 49246)
>+++ webroot/css/amo2009/main.css	(working copy)
>@@ -4137,3 +4137,17 @@
>     height: 150px;
>     width: 150px;
> }
>+
>+.home-list-contrib {

How do home-list-contrib and home-list-contrib-b1 differ?  Or maybe the question is, why are the names so similar?

>+.developersupport {
>+font-weight: bold;
>+font-size: 0.9em;
>+}

This is duplicated from above.

>Index: controllers/addons_controller.php
>===================================================================
>--- controllers/addons_controller.php	(revision 49246)
>+++ controllers/addons_controller.php	(working copy)
>@@ -554,7 +555,7 @@
>         global $app_listedtypes;
> 
>         $associations = array(
>-            'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
>+            'contrib_details', 'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
>             'latest_version', 'list_details'
>         );
>         $list_num = 5;
>@@ -957,7 +958,7 @@
> 
>         $associations = array(
>             'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
>-            'latest_version', 'list_details'
>+            'latest_version', 'list_details', 'contrib_details'
>         );
> 
>         $valid_status = array(STATUS_PUBLIC);

Caching works by joining all these words together and making that a key for the object.  If the order of the words is different, the same object will be cached under different keys.

Really, the caching piece should sort the keys alphabetically, but that's a different bug.

>Index: views/addons/category_landing.thtml
>===================================================================
>--- views/addons/category_landing.thtml	(revision 49246)
>+++ views/addons/category_landing.thtml	(working copy)
>@@ -125,6 +125,20 @@
>                                 
>                             } 
>                         ?>
>+  <?php
>+      if (isset($addon['Addon']['suggested_amount'])):
>+          if ($addon['Addon']['suggested_amount'] != 0) {

suggested_amount could also be empty.  I would use if (!empty(suggested_amount)).

>+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
>+          } else {
>+              $_title = ___('Support this add-on');
>+          }
>+  ?>
>+  
>+  <div class="home-list-contrib-bl">

Can you indent the HTML within the if?

>+  <?=$html->link($_title, "/addon/{$addonID}/developers", array('class' => 'developersupport'))?>

Can you hyphenate multi-word selectors, i.e. developer-support?  It makes them easier to read and fits the style of our other CSS.

And I would suggest using <a href...> here because I like my HTML to look like HTML, but that's me.

>+  </div>
>+  
>+  <?php endif; //['Addon']['suggested_amount'] ?>
>                     </div>
>                 </li>
>             <?php endforeach ?>
>Index: site/app/views/elements/amo2009/homepage_addon.thtml
>===================================================================
>--- site/app/views/elements/amo2009/homepage_addon.thtml	(revision 49550)
>+++ site/app/views/elements/amo2009/homepage_addon.thtml	(working copy)
>@@ -107,6 +107,20 @@
>         <?=$this->renderElement('amo2009/reviews', array('addon' => $addon))?>
>         <span class="downloads"><strong><?=$html->number_format($addonWeeklyDownloads, 0)?></strong> <?=___('weekly downloads')?></span>
>     </div>
>+    <?php
>+      if (isset($addon['Addon']['suggested_amount'])):
>+          if ($addon['Addon']['suggested_amount'] != 0) {
>+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
>+          } else {
>+              $_title = ___('Support this add-on');
>+          }
>+  ?>
>+  
>+  <div class="home-list-contrib">
>+  <?=$html->link($_title, "/addon/{$addonId}/developers", array('class' => 'developersupport'))?>
>+  </div>
>+  
>+  <?php endif; //['Addon']['suggested_amount'] ?>

This is duplicated from above.  Can you put it in an element?

Comment 16

[Milos Dinic [:Milos]]

> Can you add -webkit-border-radius and plain border-radius?

Yeah, I will.
 
> >+    border: 1px solid transparent;
> 
> If the border is transparent, do we actually need it?

Yes; if we don`t define a browser(we do with setting it's type(solid) and width), then we can`t have curved borders neither.
 
> >+.developersupport {
> >+font-weight: bold;
> >+font-size: 0.9em;
> 
> Indentation?

Will do.

> >Index: webroot/css/amo2009/main.css
> >===================================================================
> >--- webroot/css/amo2009/main.css	(revision 49246)
> >+++ webroot/css/amo2009/main.css	(working copy)
> >@@ -4137,3 +4137,17 @@
> >     height: 150px;
> >     width: 150px;
> > }
> >+
> >+.home-list-contrib {
> 
> How do home-list-contrib and home-list-contrib-b1 differ?  Or maybe the
> question is, why are the names so similar?
> 

In background colors, border-radius... They`re so similiar `cos they`re used for the same element which we display on two different pages. In this case home-list-contrib is used on homepage, and home-list-contrib-bl (blue) is used on cat landing page

> >+.developersupport {
> >+font-weight: bold;
> >+font-size: 0.9em;
> >+}
> 
> This is duplicated from above.

Yeah, my mistake. I was testing on local remora, so I used dev in conf.

> >Index: controllers/addons_controller.php
> >===================================================================
> >--- controllers/addons_controller.php	(revision 49246)
> >+++ controllers/addons_controller.php	(working copy)
> >@@ -554,7 +555,7 @@
> >         global $app_listedtypes;
> > 
> >         $associations = array(
> >-            'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
> >+            'contrib_details', 'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
> >             'latest_version', 'list_details'
> >         );
> >         $list_num = 5;
> >@@ -957,7 +958,7 @@
> > 
> >         $associations = array(
> >             'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
> >-            'latest_version', 'list_details'
> >+            'latest_version', 'list_details', 'contrib_details'
> >         );
> > 
> >         $valid_status = array(STATUS_PUBLIC);
> 
> Caching works by joining all these words together and making that a key for the
> object.  If the order of the words is different, the same object will be cached
> under different keys.
> 
> Really, the caching piece should sort the keys alphabetically, but that's a
> different bug.
> 

I can`t get the point.

> >Index: views/addons/category_landing.thtml
> >===================================================================
> >--- views/addons/category_landing.thtml	(revision 49246)
> >+++ views/addons/category_landing.thtml	(working copy)
> >@@ -125,6 +125,20 @@
> >                                 
> >                             } 
> >                         ?>
> >+  <?php
> >+      if (isset($addon['Addon']['suggested_amount'])):
> >+          if ($addon['Addon']['suggested_amount'] != 0) {
> 
> suggested_amount could also be empty.  I would use if
> (!empty(suggested_amount))

A one can actually set the Suggested amount to blank?
.
> 
> >+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
> >+          } else {
> >+              $_title = ___('Support this add-on');
> >+          }
> >+  ?>
> >+  
> >+  <div class="home-list-contrib-bl">
> 
> Can you indent the HTML within the if?
> 

I`m not sure I know the proper way of doing that.

> >+  <?=$html->link($_title, "/addon/{$addonID}/developers", array('class' => 'developersupport'))?>
> 
> Can you hyphenate multi-word selectors, i.e. developer-support?  It makes them
> easier to read and fits the style of our other CSS.

Yes

> And I would suggest using <a href...> here because I like my HTML to look like
> HTML, but that's me.
> 
> >+  </div>
> >+  
> >+  <?php endif; //['Addon']['suggested_amount'] ?>
> >                     </div>
> >                 </li>
> >             <?php endforeach ?>
> >Index: site/app/views/elements/amo2009/homepage_addon.thtml
> >===================================================================
> >--- site/app/views/elements/amo2009/homepage_addon.thtml	(revision 49550)
> >+++ site/app/views/elements/amo2009/homepage_addon.thtml	(working copy)
> >@@ -107,6 +107,20 @@
> >         <?=$this->renderElement('amo2009/reviews', array('addon' => $addon))?>
> >         <span class="downloads"><strong><?=$html->number_format($addonWeeklyDownloads, 0)?></strong> <?=___('weekly downloads')?></span>
> >     </div>
> >+    <?php
> >+      if (isset($addon['Addon']['suggested_amount'])):
> >+          if ($addon['Addon']['suggested_amount'] != 0) {
> >+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
> >+          } else {
> >+              $_title = ___('Support this add-on');
> >+          }
> >+  ?>
> >+  
> >+  <div class="home-list-contrib">
> >+  <?=$html->link($_title, "/addon/{$addonId}/developers", array('class' => 'developersupport'))?>
> >+  </div>
> >+  
> >+  <?php endif; //['Addon']['suggested_amount'] ?>
> 
> This is duplicated from above.  Can you put it in an element?

It`s not duplicated. I changed 2 pages.

Comment 17

[Milos Dinic [:Milos]]

Attachment: patch v6

Did everything, except that I still use if isset and I don`t get the thing about caching.

Comment 18

[Milos Dinic [:Milos]]

Comment on attachment 395468 [details] [diff] [review]
patch v6

>Index: webroot/css/amo2009/main-mozilla.css
>===================================================================
>--- webroot/css/amo2009/main-mozilla.css	(revision 49246)
>+++ webroot/css/amo2009/main-mozilla.css	(working copy)
>@@ -1807,9 +1807,7 @@
> 
> 
> #featured_addons div.addon_block .install-container {
>-    position: absolute;
>-    left: 7px;
>-    bottom: 7px;
>+    margin-right: 225px;
> }
> .html-rtl #featured_addons div.addon_block .install-container {
>     left: auto;
>@@ -2389,4 +2388,20 @@
> 	margin-top: .25em;
> }
> 
>+.home-list-contrib-bl {
>+    margin-top: 30px;
>+    margin-left: 10px;
>+    text-align: center;
>+    width: 70%;
>+   -moz-border-radius: 8px;
>+    background-color: #DFEEF5;
>+   -webkit-border-radius: 8px;
>+    border-radius: 8px;
>+}
>+
>+.developer-support {
>+font-weight: bold;
>+font-size: 0.9em;
>+}
>+
> /** END Login Forms **/
>Index: webroot/css/amo2009/main.css
>===================================================================
>--- webroot/css/amo2009/main.css	(revision 49246)
>+++ webroot/css/amo2009/main.css	(working copy)
>@@ -4137,3 +4137,11 @@
>     height: 150px;
>     width: 150px;
> }
>+
>+.home-list-contrib {
>+padding-top: 3px;
>+text-align: center;
>+width: 100%;
>+-moz-border-radius: 3px;
>+background-color: #F7F6D3;
>+}
>Index: controllers/addons_controller.php
>===================================================================
>--- controllers/addons_controller.php	(revision 49246)
>+++ controllers/addons_controller.php	(working copy)
>@@ -554,7 +555,7 @@
>         global $app_listedtypes;
> 
>         $associations = array(
>-            'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
>+            'contrib_details', 'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
>             'latest_version', 'list_details'
>         );
>         $list_num = 5;
>@@ -957,7 +958,7 @@
> 
>         $associations = array(
>             'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
>-            'latest_version', 'list_details'
>+            'latest_version', 'list_details', 'contrib_details'
>         );
> 
>         $valid_status = array(STATUS_PUBLIC);
>Index: views/addons/category_landing.thtml
>===================================================================
>--- views/addons/category_landing.thtml	(revision 49246)
>+++ views/addons/category_landing.thtml	(working copy)
>@@ -125,6 +125,20 @@
>                                 
>                             } 
>                         ?>
>+  <?php
>+      if (!empty($addon['Addon']['suggested_amount'])):
>+          if ($addon['Addon']['suggested_amount'] != 0) {
>+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
>+          } else {
>+              $_title = ___('Support this add-on');
>+          }
>+  ?>
>+  
>+      <div class="home-list-contrib-bl">
>+          <?=$html->link($_title, "/addon/{$addonID}/developers", array('class' => 'developer-support'))?>
>+      </div>
>+  
>+  <?php endif; //['Addon']['suggested_amount'] ?>
>                     </div>
>                 </li>
>             <?php endforeach ?>
>Index: site/app/views/elements/amo2009/homepage_addon.thtml
>===================================================================
>--- site/app/views/elements/amo2009/homepage_addon.thtml	(revision 49550)
>+++ site/app/views/elements/amo2009/homepage_addon.thtml	(working copy)
>@@ -107,6 +107,20 @@
>         <?=$this->renderElement('amo2009/reviews', array('addon' => $addon))?>
>         <span class="downloads"><strong><?=$html->number_format($addonWeeklyDownloads, 0)?></strong> <?=___('weekly downloads')?></span>
>     </div>
>+    <?php
>+      if (isset($addon['Addon']['suggested_amount'])):
>+          if ($addon['Addon']['suggested_amount'] != 0) {
>+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
>+          } else {
>+              $_title = ___('Support this add-on');
>+          }
>+  ?>
>+  
>+      <div class="home-list-contrib">
>+          <?=$html->link($_title, "/addon/{$addonId}/developers", array('class' => 'developer-support'))?>
>+      </div>
>+  
>+  <?php endif; //['Addon']['suggested_amount'] ?>
> </div>
> 
> <?php if (false): ?>
>

Comment 19

[Milos Dinic [:Milos]]

Attachment: patch v7

fixed

Comment 20

[Jeff Balogh (:jbalogh)]

(In reply to comment #16)
> > >+    border: 1px solid transparent;
> > 
> > If the border is transparent, do we actually need it?
> 
> Yes; if we don`t define a browser(we do with setting it's type(solid) and
> width), then we can`t have curved borders neither.

We have other curved boxes without borders, like the blue boxes on the side of addon and collection pages.

> > >Index: webroot/css/amo2009/main.css
> > >===================================================================
> > >--- webroot/css/amo2009/main.css	(revision 49246)
> > >+++ webroot/css/amo2009/main.css	(working copy)
> > >@@ -4137,3 +4137,17 @@
> > >     height: 150px;
> > >     width: 150px;
> > > }
> > >+
> > >+.home-list-contrib {
> > 
> > How do home-list-contrib and home-list-contrib-b1 differ?  Or maybe the
> > question is, why are the names so similar?
> > 
> 
> In background colors, border-radius... They`re so similiar `cos they`re used
> for the same element which we display on two different pages. In this case
> home-list-contrib is used on homepage, and home-list-contrib-bl (blue) is used
> on cat landing page

So maybe the second one should be category-landing-contrib?

Or better yet, the homepage has class="home" and category pages have class="category_landing", so you can name the div you're adding "support-this-addon" and use .home .support-this-addon and .category_landiing .support-this-addon to differentiate.

> > >Index: controllers/addons_controller.php
> > >===================================================================
> > >--- controllers/addons_controller.php	(revision 49246)
> > >+++ controllers/addons_controller.php	(working copy)
> > >@@ -554,7 +555,7 @@
> > >         global $app_listedtypes;
> > > 
> > >         $associations = array(
> > >-            'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
> > >+            'contrib_details', 'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
> > >             'latest_version', 'list_details'
> > >         );
> > >         $list_num = 5;
> > >@@ -957,7 +958,7 @@
> > > 
> > >         $associations = array(
> > >             'single_category', 'all_categories', 'authors', 'compatible_apps', 'files',
> > >-            'latest_version', 'list_details'
> > >+            'latest_version', 'list_details', 'contrib_details'
> > >         );
> > > 
> > >         $valid_status = array(STATUS_PUBLIC);
> > 
> > Caching works by joining all these words together and making that a key for the
> > object.  If the order of the words is different, the same object will be cached
> > under different keys.
> > 
> > Really, the caching piece should sort the keys alphabetically, but that's a
> > different bug.
> > 
> 
> I can`t get the point.

The order of items in the $associations array should be the same so they're cached in the same place.

> > >Index: views/addons/category_landing.thtml
> > >===================================================================
> > >--- views/addons/category_landing.thtml	(revision 49246)
> > >+++ views/addons/category_landing.thtml	(working copy)
> > >@@ -125,6 +125,20 @@
> > >                                 
> > >                             } 
> > >                         ?>
> > >+  <?php
> > >+      if (isset($addon['Addon']['suggested_amount'])):
> > >+          if ($addon['Addon']['suggested_amount'] != 0) {
> > 
> > suggested_amount could also be empty.  I would use if
> > (!empty(suggested_amount))
> 
> A one can actually set the Suggested amount to blank?

Yes, it could happen.  It's not supposed to, but you never know.

> > >+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
> > >+          } else {
> > >+              $_title = ___('Support this add-on');
> > >+          }
> > >+  ?>
> > >+  
> > >+  <div class="home-list-contrib-bl">
> > 
> > Can you indent the HTML within the if?
> > 
> 
> I`m not sure I know the proper way of doing that.

Just put another 4 spaces before the HTML inside the <?php if?> just like you're indenting the php code.


> > >+  </div>
> > >+  
> > >+  <?php endif; //['Addon']['suggested_amount'] ?>
> > >                     </div>
> > >                 </li>
> > >             <?php endforeach ?>
> > >Index: site/app/views/elements/amo2009/homepage_addon.thtml
> > >===================================================================
> > >--- site/app/views/elements/amo2009/homepage_addon.thtml	(revision 49550)
> > >+++ site/app/views/elements/amo2009/homepage_addon.thtml	(working copy)
> > >@@ -107,6 +107,20 @@
> > >         <?=$this->renderElement('amo2009/reviews', array('addon' => $addon))?>
> > >         <span class="downloads"><strong><?=$html->number_format($addonWeeklyDownloads, 0)?></strong> <?=___('weekly downloads')?></span>
> > >     </div>
> > >+    <?php
> > >+      if (isset($addon['Addon']['suggested_amount'])):
> > >+          if ($addon['Addon']['suggested_amount'] != 0) {
> > >+              $_title = sprintf(___('Support this add-on: Contribute $%1$s'), $addon['Addon']['suggested_amount']);
> > >+          } else {
> > >+              $_title = ___('Support this add-on');
> > >+          }
> > >+  ?>
> > >+  
> > >+  <div class="home-list-contrib">
> > >+  <?=$html->link($_title, "/addon/{$addonId}/developers", array('class' => 'developersupport'))?>
> > >+  </div>
> > >+  
> > >+  <?php endif; //['Addon']['suggested_amount'] ?>
> > 
> > This is duplicated from above.  Can you put it in an element?
> 
> It`s not duplicated. I changed 2 pages.

But you have the same code on 2 different pages, which means we'll have to update 2 places if we want to change this in the future.  Putting this in an element means we only have one copy.

See  views/elements/amo2009/collections/recently_viewed.thtml for an example, used in collections/detail and collections/listing.

Comment 21

[Jeff Balogh (:jbalogh)]

Attachment: Contribute element in the center

The mock shows the contribute button on the bottom, but we currently have the install button pinned to the bottom of the box.

I tried moving the install button up, but that makes the page look weird and busy because the install buttons are not in a consistent place; they end up wherever the text description ends.

In the screenshot I'm leaving the install button alone and moving the contribute element to the center.  I like the look of neilio's mock better, but it doesn't work when you put it on a page with varying text lengths.  What do you think about this way?

Comment 22

[Jeff Balogh (:jbalogh)]

Attachment: contribution amounts on home, category, search pages

Building off of Milos' patch, plus some other cleanups I ran into: normalizing $associations in addons_controller and deleting some dead code that was commented out with an if(false).

The category landing page strays from the mock to make it work, pending nick's approval.

Comment 23

[Wil Clouser [:clouserw]]

Comment on attachment 395505 [details] [diff] [review]
contribution amounts on home, category, search pages

Even with this patch, neither looks like the mockup.

http://www.pict.com/view/1495994/0/addons2520for2520firefox

It's hard to see in the mockup, but I think that is supposed to be centered in the full width.  Even if it's not, if you make the window as small as the mockup it wraps onto multiple lines.

http://www.pict.com/view/1495990/0/alerts2520262520updates25203a3a2
I know you moved this on purpose (which is fine, I think) but it's not displaying correctly for me.

Comment 24

[Jeff Balogh (:jbalogh)]

Attachment: In total, this is v9!

Comment 25

[Jeff Balogh (:jbalogh)]

r49641

Comment 26

[Jeff Balogh (:jbalogh)]

Thanks Milos, this was easy to finish up based on your patches!

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on:

* https://addons.mozilla.org/en-US/firefox/browse/type:1/cat:72
* https://addons.mozilla.org/en-US/firefox/
* https://addons.mozilla.org/en-US/firefox/search?q=febe&cat=all