594795 - Add a "What's an Add-on" block to the Add-ons Manager

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

We've wanted to have some explanation of what add-ons are, for new users, in the product for a while.  The Add-ons Manager redesign for desktop Firefox 4 includes a section with a brief explanation and a button that leads to a more informative webpage.  We should include a slimmed down version of this section in the Fennec Add-ons Manager.

Here's a mockup:
http://www.flickr.com/photos/madhava_work/4971673433/sizes/l/

For reference, here's what the desktop version looks like:
http://www.flickr.com/photos/madhava_work/4971680897/in/photostream/

We'll need for the "Learn More" button to point somewhere; a mobilized version of the one in production for desktop firefox would be ideal.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I have a patch that does this:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-what-are-addons-01.png
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-what-are-addons-02.png

I need the colors for the backgrounds.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

sub text is now black too. just need the background color/style

Comment 3

[Madhava Enros [:madhava]]

We'll need to have some way to dismiss the row, too, so it's not permanent.

Brainstorm:
- a "done" button next to "Learn More"
- close button that looks like a tab close button at right, learn more moved to left

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Do we need to dismiss it? It collapses to a small-ish row. We would remove it when viewing a search result.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

This patch is the same as the previous, with the following additions:
* Strings added to .properties file
* Added support for bug 596594
** Breaks apart the displaySearchResults into displayRecommendedResults and displaySearchResult - instead of trying to make the single function more complex.
** Does two search requests for the recommended list: the recommended items and the top X items sorted by rating

This implements much of the plan for bug 596594. We can tweak the specifics in future bugs.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 475716 [details] [diff] [review]
patch 2

>diff --git a/chrome/content/bindings/extensions.xml b/chrome/content/bindings/extensions.xml
>--- a/chrome/content/bindings/extensions.xml
>+++ b/chrome/content/bindings/extensions.xml
>@@ -236,9 +236,30 @@

>+  <binding id="extension-search-banner" extends="chrome://browser/content/bindings.xml#richlistitem">
>+    <content orient="vertical" nohighlight="true">

Nit: I'm not sure why I've missed that last time but "nohighlight" seems a crazy name to me.
I would rather prefer "ignorehighlight"

>+  appendSearchResults: function(aAddons, aShowRating) {

Nit: let's keep the nice debugging name that are everywhere into this file
>+  displayRecommendedResults: function ev_displaySearchResults(aRecommendedAddons, aBrowseAddons) {
>+    this.clearSection("repo");
>+    let whatare = document.createElement("richlistitem");
>+    whatare.setAttribute("typeName", "banner");
>+
>+    whatare.setAttribute("label", strings.getString("addonsWhatAre.label"));

Nit: no need for the middle line break

>+///////////////////////////////////////////////////////////////////////////////
>+// callback for the browse search
>+var BrowseSearchResults = {
>+  cache: null,
>+
>+  searchSucceeded: function(aAddons, aAddonCount, aTotalResults) {
>+    this.cache = aAddons;
>+    ExtensionsView.displayRecommendedResults(RecommendedSearchResults.cache, aAddons);
>   },

What is the "cache" property for?


>diff --git a/themes/core/platform.css b/themes/core/platform.css
>--- a/themes/core/platform.css
>+++ b/themes/core/platform.css
>@@ -545,16 +545,22 @@ richlistitem description.title {
> 
> richlistitem label.normal,
> richlistitem description.normal {
>   color: gray;
>   font-size: 18px !important;
>   white-space: pre-wrap;
> }
> 
>+richlistitem label.normal-black,
>+richlistitem description.normal-black {
>+  font-size: 18px !important;
>+  white-space: pre-wrap;
>+}
>+

The name is a bit misleading since normal-black sounds like this is the normal style + black text but 
in fact this is just normal

Maybe we should replace this by normal and switch "normal" to "normal-gray"?



r+ with nits adressed.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)

> >+  <binding id="extension-search-banner" extends="chrome://browser/content/bindings.xml#richlistitem">
> >+    <content orient="vertical" nohighlight="true">
> 
> Nit: I'm not sure why I've missed that last time but "nohighlight" seems a
> crazy name to me.
> I would rather prefer "ignorehighlight"

We use this in other places so we should file a new bug to change it

> >+  appendSearchResults: function(aAddons, aShowRating) {
> 
> Nit: let's keep the nice debugging name that are everywhere into this file

I'll add the name

> >+  displayRecommendedResults: function ev_displaySearchResults(aRecommendedAddons, aBrowseAddons) {
> >+    this.clearSection("repo");
> >+    let whatare = document.createElement("richlistitem");
> >+    whatare.setAttribute("typeName", "banner");
> >+
> >+    whatare.setAttribute("label", strings.getString("addonsWhatAre.label"));
> 
> Nit: no need for the middle line break

Will remove

> >+var BrowseSearchResults = {
> >+  cache: null,
> >+
> >+  searchSucceeded: function(aAddons, aAddonCount, aTotalResults) {
> >+    this.cache = aAddons;
> 
> What is the "cache" property for?

Nothing. I'll remove it

> >+richlistitem label.normal-black,
> >+richlistitem description.normal-black {

> The name is a bit misleading since normal-black sounds like this is the normal
> style + black text but 
> in fact this is just normal
> 
> Maybe we should replace this by normal and switch "normal" to "normal-gray"?

I'll file a new bug

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/9bfef4e850e6

Comment 9

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b6pre) Gecko/20100930 Namoroka/4.0b7pre Fennec/4.0b1pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100930 Namoroka/4.0b7pre Fennec/4.0b1pre


Follow-up bug: https://bugzilla.mozilla.org/show_bug.cgi?id=600862

Comment 10

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=13630 created to regression test this bug