Closed Bug 428033 Opened 16 years ago Closed 16 years ago

Pull apart "Themes" and "Appearance" categories

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: madhava, Assigned: wenzel)

Details

Attachments

(3 files, 4 obsolete files)

We decided to do this back in the 3.2 preview timeframe, but it's worth having a bug to track it.  Themes and "appearance" extensions shouldn't be in the same category.  There should be a category called Themes dedicated to just themes.

As a side issue, there are all sorts of problems with the "Appearance" category - most are there for the wrong reasons), but we shouldn't postpone having a distinct Themes category until we've sorted out what to do with the Appearance one.  In the short term, we might as well have a top level "Appearance" category until we figure out what to do with it.
Target Milestone: 3.2.1 → 3.4
Assignee: nobody → fwenzel
Target Milestone: 3.4 → 3.4.1
This is the first part of the process: De-hybridizing Themes/Appearance and Lang Tools pages. This patch does that and adds these add-on types to the main navigation menu.

Note that navigation is going to change further down the road to AMO 3.4, so the new category links are not going to be part of the flat list forever.

Also note that this bug does not fix any of the landing pages yet.
Attachment #317308 - Flags: review?(laura)
Comment on attachment 317308 [details] [diff] [review]
De-hybridize part 1: navigation links

"Themes" is missing from the search drop down.
This patch adds Plugins to the navigation links, too. Morgamic, could you please take a look at this?
Attachment #317308 - Attachment is obsolete: true
Attachment #317995 - Flags: review?(morgamic)
Attachment #317308 - Flags: review?(laura)
Now of course I forgot to restrict the whole plugins thing by application. This patch is better :)
Attachment #317995 - Attachment is obsolete: true
Attachment #317998 - Flags: review?(morgamic)
Attachment #317995 - Flags: review?(morgamic)
Comment on attachment 317998 [details] [diff] [review]
De-hybridizing nav links (rev3): Include Plugins link *for appropriate apps only*

Works for me. A few nits:

>Index: controllers/components/amo.php
...
>-?>
Not sure why this isn't throwing a parse error, but we should probably keep it in.

>Index: views/elements/categories.thtml
...
>+        echo '<li>', $html->link($html->entities($_tag['name']), $_url), "</li>\n";
As far as I know we haven't used commas instead of concatenation anywhere else - should probably be consistent.
Attachment #317998 - Flags: review?(morgamic) → review+
Okay, will change these. Thanks for the review, fligtar.

Side notes: Opening PHP tags do not need to be closed, and end at the end of the file implicitly. Also, echo's comma syntax is likely faster and nicer on memory than concatenation. But I guess it makes little sense to only use it in one place.
All right, the nav changes (incl. fligtar's remarks) are in r12587. Will follow up with further patches.
Attached patch Part 2: Fixing search (obsolete) — Splinter Review
The search dropdown is now filled with the new category list and the search code uses both the add-on type and the category to restrict the results.
Attachment #318129 - Flags: review?(fligtar)
Attachment #318129 - Attachment is obsolete: true
Attachment #318130 - Flags: review?
Attachment #318129 - Flags: review?(fligtar)
Comment on attachment 318130 [details] [diff] [review]
Fixing search, search component included

I forgot to include the search component in the diff. :-/
Attachment #318130 - Flags: review? → review?(fligtar)
Comment on attachment 318130 [details] [diff] [review]
Fixing search, search component included

r=fligtar

I've been noticing a lot of display bugs but have been assuming they'll be fixed in a later patch, so just tell me when it's the last patch so I can test the stuff I'm seeing.
Attachment #318130 - Flags: review?(fligtar) → review+
Yeah it's true, it's not *quite* done yet, but we're getting there. The search patch is in r12609, thanks for the review.
Attached patch Part 3: Themes page (obsolete) — Splinter Review
This one is for the actual themes page and includes smaller fixes to the category list too.

This is the last patch in the series. So if you see anything broken after applying it, please let me know. I can then provide another patch or fix this one as needed.
Attachment #318376 - Flags: review?(fligtar)
Comment on attachment 318376 [details] [diff] [review]
Part 3: Themes page

Everything's good except that this broke search for me. I get a blank page with a <div> when I try a search.

Few other things:
- When selecting the Bookmarks category from the sidebar, the search dropdown is pre-filled with "Themes" for some reason. This only seems to happen for the Bookmarks category.

- The Plugins landing page could use some prettiness. (maybe a different bug)
Attachment #318376 - Flags: review?(fligtar) → review-
I reverted each file and found that the AmoComponent change is what broke search, fwiw.
Thanks, Fligtar. The Addontype model wasn't loaded in the search controller, which I missed.

I fixed the Bookmarks category problem too.

The Plugins page ugliness is indeed a different bug, which I am pretty sure nobody has filed yet, but I agree; at the very least it doesn't match the other pages' layout. Wanna file it? :)
Status: NEW → ASSIGNED
new patch, including the aforementioned fixes.
Attachment #318376 - Attachment is obsolete: true
Attachment #318416 - Flags: review?(fligtar)
Comment on attachment 318416 [details] [diff] [review]
Themes de-hyb., rev2

Looks good.
Attachment #318416 - Flags: review?(fligtar) → review+
Filed bug 431342 on plugins page.
The patch is in r12640. If any additional problems occur with this feel free to reopen or file a new bug.

Thanks, everybody.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: push-needed
Resolution: --- → FIXED
Fred: http://img120.imageshack.us/my.php?image=picture1hu9.png.

There, "Themes & Appearance" is displayed as a single category.

[1] https://preview.addons.mozilla.org/en-US/firefox/browse/type:1/cat:14?sort=updated
[2] https://preview.addons.mozilla.org/en-US/firefox/search?q=splash&cat=all

I at first thought that this might be due to some needed database/CP-type tweaking, but "Appearance" shows up on those pages for certain add-ons...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #21)
> Fred: http://img120.imageshack.us/my.php?image=picture1hu9.png.
> 
> There, "Themes & Appearance" is displayed as a single category.

You guessed it.... -- it's a caching issue:

https://preview.addons.mozilla.org/en-US/firefox/addon/2995

Shows "Appearance" correctly.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We're still seeing this on the links in comment #21.  It's not in the dropdown category list, but in the actual add-on listing itself.
While I still see it too, I am wondering how that's even technically possible. If somebody has a great idea, speak up :)
(In reply to comment #24)
> While I still see it too, I am wondering how that's even technically possible.
> If somebody has a great idea, speak up :)

Are we planning on shipping with this, then, and addressing in 3.4.2?
(In reply to comment #24)
> While I still see it too, I am wondering how that's even technically possible.
> If somebody has a great idea, speak up :)
> 

I changed the name of the category on my local copy and it works fine...
(In reply to comment #25)
> (In reply to comment #24)
> > While I still see it too, I am wondering how that's even technically possible.
> > If somebody has a great idea, speak up :)
> 
> Are we planning on shipping with this, then, and addressing in 3.4.2?
> 

Let's talk to Jeremy tomorrow about flushing all the caches, etc. on preview and verifying the db is correct and not corrupt.  As of now, it's a bug I can't reproduce locally.
Preview wasn't using a MEMCACHE_PREFIX which caused this.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Keywords: push-needed
Verified FIXED on production too, anyways.  (https://addons.mozilla.org/en-US/firefox/search?q=splash&cat=all to address 21.)

:-)
Status: RESOLVED → VERIFIED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: