Closed Bug 490889 Opened 16 years ago Closed 16 years ago

Add the new header/footer to AMO

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: clouserw, Assigned: lorchard)

References

Details

Attachments

(6 files, 2 obsolete files)

We have a fancy new header/footer from clearleft that we need to put on the site. Code coming asap.
Blocks: 490897
Blocks: 490896
Blocks: 490887
Whiteboard: ->rdoherty
Assignee: nat → lorchard
Severity: critical → normal
Priority: -- → P1
Here's a patch with initial work on getting the new header/footer and CSS working. Some notes: * The new layout is called "amo2009" and can be switched to in controllers by swapping $this->layout="mozilla" for $this->layout="amo2009" * I've got all the new images/CSS/JS isolated under site/app/webroot/amo2009 in order to ease transitional co-existence with the existing site layout. * The new layout does not include any CSS from the old layout. * There's a file site/app/webroot/amo2009/css/legacy.css - the intention for this file is that whenever styles are needed from the pre-AMO-2009 layout, copy them here. Hopefully that will enable a cleaner transition, pulling over just the styles that are needed, rather than needing to identify and clean out a bunch of old styles whenever we're ready to switch completely over to the new theme. * There's a new directory site/app/views/elements/amo2009 that contains elements for the new search bar and category list. I've started using these in implementing the new home page design for bug 490906. They should be useful on other pages, and it looks like the design uses the same category list HTML for pages with expanded or clickable category lists. I'm sure that this patch will need continuing tweaks, but hopefully this gives other bugs a headstart.
Attachment #376733 - Flags: review?(clouserw)
In case it's helpful, I've posted this patch on bug 490906: https://bug490906.bugzilla.mozilla.org/attachment.cgi?id=376734 It shows a few changes to enable the amo2009 layout, as well as some tweaks to page structure to include the search bar and category list. Other pages will have further structural changes
Thanks Les, this should be helpful. I'm seeing some weirdness in the patch with our good friends the whitespace characters. There's some ^M's strewn about (search for "line-height:1.538em"), and are those (GASP!) tabs?
I'm pretty sure those are from Clearleft, since I tried to touch their CSS as little as possible beyond a search/replace on paths. I can take a sweep through to clean those up, but I didn't want to muck up any styles
Hello, I believe any ^M 's you are seeing must be encoding issues because we don't have them this end. I'm afraid those are tabs you are seeing :) it's a house style. Kind regards Natalie
Quick tweak to the previous path, making it easier to use Cake helpers to load AMO 2009 assets. Also fixed the ^M's and tabs in main.css
Attachment #376733 - Attachment is obsolete: true
Attachment #376815 - Flags: review?(jbalogh)
Attachment #376815 - Flags: review?(clouserw)
Attachment #376733 - Flags: review?(clouserw)
Since the git image diffs seem not-so-usable, here's a tarball of the images
Attachment #376815 - Flags: review?(jbalogh) → review+
Comment on attachment 376815 [details] [diff] [review] Swapping amo2009/{css,js,img} paths for {css,js,img}/amo2009 paths Do we want ___() on those? I know, they weren't there before. diff --git a/site/app/views/elements/amo2009/categories.thtml b/site/app/views/elements/amo2009/categories.thtml > new file mode 100644 > index 0000000..a637fb4 > --- /dev/null > +++ b/site/app/views/elements/amo2009/categories.thtml > @@ -0,0 +1,58 @@ > +<?php > + <ul> > + <li> > + <ul> Any idea what the list within a list is for? > diff --git a/site/app/views/elements/amo2009/search.thtml b/site/app/views/elements/amo2009/search.thtml > new file mode 100644 > index 0000000..eec7f63 > --- /dev/null > +++ b/site/app/views/elements/amo2009/search.thtml > @@ -0,0 +1,323 @@ > +<?php > +/** > + * This element uses the following local variables: > + * $query: query string to be displayed in the text box, defaults to "search for add-ons" > + * $category: category ID to be selected > + */ > +<div class="search-form expanded-search-form"> > + <form method="get" action="<?=$html->url("/search")?>"> > + <div class="basic"> > + <input type="text" title="search" name="q" /> The old version has value="<?=$query?>". Do we still want that? diff --git a/site/app/views/layouts/amo2009.thtml b/site/app/views/layouts/amo2009.thtml > new file mode 100644 > index 0000000..23c741a > --- /dev/null > +++ b/site/app/views/layouts/amo2009.thtml > @@ -0,0 +1,393 @@ > + > +$bodyclass = 'home'; > +$bodyclass = 'html-'.TEXTDIR.' firefox '.$bodyclass; > + > +$bodyclass = (isset($bodyclass) && $bodyclass != '')?' class="'.$bodyclass.'"':""; The above lines suggest that $bodyclass will always be set. > +$htmlclass = (isset($htmlclass) && $htmlclass != '')?' class="'.$htmlclass.'"':""; > +$headertitle = (isset($headertitle) && $headertitle != '')?$headertitle:"Pattern Portfolio"; > +$header_extra = (isset($header_extra) && $header_extra != '')?$header_extra:""; I'd love to see that repetition in a function. > +<body id="mozilla-com" <?php echo $bodyclass ?>> > + <ul id="nav-access"> > + <li><a href="#content">Skip to main content</a></li> > + <li><a href="#search-form">Skip to search form</a></li> > + <li><a href="#categories">Skip to categories menu</a></li> > + <li><a href="#other-apps">Skip to other applications menu</a></li> > + </ul> Do we want ___() on those? I know, they weren't there before. > + // HACK: If "Firefox" appears in the localized main header, replace > + // it with the image. > + $main_header = str_replace( > + 'Firefox', > + '<img src="/amo2009/img/logo-firefox.gif" alt="Firefox" />', I think you've fixed this already locally. > + $main_header > + ); > + ?> > + <p id="title"><a href="http://addons.mozilla.com/"><?= $main_header ?></a></p> > + <p id="brand"><a href="http://mozilla.com/">Mozilla</a></p> ___()? > + <ul class="tools"> > + <li> > + <a href="#" class="controller">Tools</a> ___()? > +<div id="footer" role="contentinfo"> > + <div class="section"> > + <div class="primary"> > + > + <form class="languages"> > + <label for="language"><?=_('footer_other_languages')?></label> > + <select id="language" name="lang" dir="ltr" onchange="this.form.submit()"> > + <?php > + // Retrieve language arrays from bootstrap. > + global $supported_languages, $native_languages; > + foreach (array_keys($supported_languages) as $key) { > + echo '<option value="'.$key.'" '.($key == LANG ? 'selected="selected"' : '').'>' > + .$native_languages[$key]['native'].'</option>'."\n"; > + } > + ?> > + </select> > + <button><?=_('footer_lang_form_lang_submit_go')?></button> Is it a usability thing to have the button there all the time, even though the form gets submitted onchange with javascript?
Hello, Just a quick point, any php in my delivered code is purely demonstrative of the HTML and is definately not designed for production. I hope this helps some of your issues above. To answer the button question, yes it is important to have a button even when the form is submitted by javascript. Kind regards, Natalie
Comment on attachment 376815 [details] [diff] [review] Swapping amo2009/{css,js,img} paths for {css,js,img}/amo2009 paths I think it's a good enough start to commit. Comments from me: - we don't need the example images in the tree (e.g. screenshot-addon-example-*). There may be others as well - what is $headertitle in amo2009.thtml? - There is a chunk of code (about 25 lines) copied from header.thtml. Everything about $url_format, $homeText, $homeLink, $publicUrl, $sandboxUrl, and $sandboxLink. Are those variables actually used anywhere? I couldn't find them. - I filed bug 492644 to make sure we compress the CSS/JS before production - is ie6.css ever used? I only see ie.css included. - jQuery 1.3.2 is already in /js/jquery-compressed.js - This URL is wrong: <img src="/amo2009/img/logo-firefox.gif" alt="Firefox" /> - Should our nav-access <ul> have role="navigation"? - The check for $supporessLanguageSelector should be added around the <form> in the footer - The language selector form is missing method and action attributes
Attachment #376815 - Flags: review?(clouserw) → review+
Checked in r25605 with a few further tweaks to enable counts in the category list, as well as to make the image replacement in title more specific. Just to be clear - I'm not closing this bug because I'm not done yet. I just wanted to get something done enough to un-block the rest of the bugs dependent on it. This is a rough combination of the original mozilla.thtml layout with the clearleft work, but there's still work to be done in localization and general clean-up that I plan to do as part of getting bug 490906 done. I expect other bugs implementing the clearleft designs will raise further issues.
Additionally, this breaks any page that doesn't have Addon set (like everything in /pages/). Adding Addon to app_controller fixes the problem but it feels like there would be a performance consequence to that.
(In reply to comment #12) > Created an attachment (id=377051) [details] > Screenshot of potential header fallout? I checked in r25607 to revert the localized string change I made to the header title. I'll have to come up with a different way to implement that header. Ideally, the code for this header/footer layout shouldn't affect any pages not switched to using it. (In reply to comment #13) > Additionally, this breaks any page that doesn't have Addon set (like everything > in /pages/). Adding Addon to app_controller fixes the problem but it feels > like there would be a performance consequence to that. Can you be more specific? What breaks, how's it broken, and what could be done do fix it? This is my first time mucking around at this level of the project, so I'm not familiar with everything.
> (In reply to comment #13) > > Additionally, this breaks any page that doesn't have Addon set (like everything > > in /pages/). Adding Addon to app_controller fixes the problem but it feels > > like there would be a performance consequence to that. > > Can you be more specific? What breaks, how's it broken, and what could be done > do fix it? This is my first time mucking around at this level of the project, > so I'm not familiar with everything. Yeah, sorry. I was multitasking! The broken stuff is anything under /pages/, e.g.: https://preview.addons.mozilla.org/en-US/firefox/pages/developer_faq It's broken because the amo component's getNavCategories() is called for every page in app_controller (every controller's parent) and you added a call to $this->controller->Addon->countAddonsInAllCategories() to that function. If the controller isn't using the Addon model, that's not an object and is a fatal error. Adding "Addon" to the $uses array in app_controller fixes this, but I'm not sure if that will mean cake decides to start joining random things to the addon model and slowing down our queries.
(In reply to comment #8) > > +++ b/site/app/views/elements/amo2009/categories.thtml > Any idea what the list within a list is for? The category list HTML/CSS is set up to allow multiple sections with dividers between them. Right now, I only have the straight list of categories. We could in the (near?) future use the separate sections for types (eg. addons, plugins, themes) vs categories. Not sure what the plan is there, though. > > +++ b/site/app/views/elements/amo2009/search.thtml > The old version has value="<?=$query?>". Do we still want that? Added this to my local copy, should appear in a patch on bug 490906 soonish. > > +++ b/site/app/views/layouts/amo2009.thtml > > @@ -0,0 +1,393 @@ > > The above lines suggest that $bodyclass will always be set. > ... > I'd love to see that repetition in a function. That bodyclass/htmlclass stuff was copy/pasted from the clearleft PHP so I could get things working. I plan on cleaning it up. There's also a pile of strings that need localizing. I tried to match up most strings in the new HTML with existing localized strings, but there are a bunch that don't match 1:1 though. Hoping to wrap all those in proper ___() calls shortly. (In reply to comment #10) > - we don't need the example images in the tree (e.g. > screenshot-addon-example-*). There may be others as well Yeah, I'll probably delete those as part of optimizations for JS / CSS / image sprites in bug 492644. > - what is $headertitle in amo2009.thtml? Junk from the original PHP. > - There is a chunk of code (about 25 lines) copied from header.thtml. > Everything about $url_format, $homeText, $homeLink, $publicUrl, $sandboxUrl, > and $sandboxLink. Are those variables actually used anywhere? I couldn't find > them. More junk to clean out. It looked important at the time. > - is ie6.css ever used? I only see ie.css included. Looks like it's not included in any of the clearleft HTML. I'll see how things look in IE6 before deleting it though. > - jQuery 1.3.2 is already in /js/jquery-compressed.js Switching to that one - wasn't sure what the version on our existing jquery was, so wanted to see the clearleft stuff working as-is first. > - This URL is wrong: <img src="/amo2009/img/logo-firefox.gif" alt="Firefox" /> Still sorting out how best to render that header that's friendly to localization... that path was fixed, and then I yanked out the localized string using it. > - Should our nav-access <ul> have role="navigation"? Adding it. > - The check for $supporessLanguageSelector should be added around the <form> in > the footer > - The language selector form is missing method and action attributes Adding both. Will probably roll further changes affecting this bug into a patch for bug 490906, and eventually close this one.
(In reply to comment #15) > Yeah, sorry. I was multitasking! The broken stuff is anything under /pages/, > e.g.: https://preview.addons.mozilla.org/en-US/firefox/pages/developer_faq > > It's broken because the amo component's getNavCategories() is called for every > page in app_controller (every controller's parent) and you added a call to > $this->controller->Addon->countAddonsInAllCategories() to that function. If > the controller isn't using the Addon model, that's not an object and is a fatal > error. Adding "Addon" to the $uses array in app_controller fixes this, but I'm > not sure if that will mean cake decides to start joining random things to the > addon model and slowing down our queries. Hmm. I wonder if those countAddonsInCategory / countAddonsInAllCategories methods might be happier living in the Tag model, which was already loaded by getNavCategories() before I started poking around. Those two methods do straight SQL queries and they're not dependent on anything in the Addon model in particular. That, or maybe I could disable the counting as implemented now. Think about adding a "count" column to Tags and update it with a maintenance script.
> That, or maybe I could disable the counting as implemented now. Think about > adding a "count" column to Tags and update it with a maintenance script. I like that!
(In reply to comment #18) > > That, or maybe I could disable the counting as implemented now. Think about > > adding a "count" column to Tags and update it with a maintenance script. > > I like that! Just made a quick check in for r25644 to disable the counting, in order to un-break things on preview. Will file a separate bug to generate category counts and the home page download stats in a maint script
I just noticed that a CSS class I want to use ('.significant') isn't in our main.css, but it is in http://clearleft.com/svn/repo/mozilla/addons/dev/css/main.css. I guess we need to keep pulling this in as natbat iterates on it?
Yeah, until this friday it'll still be changing a bit.
Whiteboard: ->rdoherty
Okay - hopefully, this is the patch that lets me call this bug closed. I also did an svn up on the checkout I have from clearleft, tweaked the main.css for tabs/spaces and image paths, copied new images over. Not all of these changes in the CSS impact the header/footer for this bug directly. Not sure what we should do going into the future to keep our stuff updated with ongoing clearleft work. Do we get updates as to when it's safe to pull in their work? Do we pull in updates as part of closing other bugs? I wish the CSS was split up into separate files related to sections or whatnot, might make the work a little more manageable as we're splitting it up on our side. We don't really need one big CSS file for performance reasons, since bug 492644 will introduce CSS minification anyway. At any rate - hope this patch does it for this bug for now.
Attachment #377426 - Flags: review?(jbalogh)
Attachment #377426 - Flags: review?(clouserw)
And here's a tarball of images embedded in the previous patch
Whoops. Accidentally checked this in as r25706 then reverted as r25709 in the course of trying to check in (r25705) (and then reverting (r25707)) another patch. Wheee, learning git svn!
Attachment #377426 - Flags: review?(clouserw) → review+
> Okay - hopefully, this is the patch that lets me call this bug closed. > > I also did an svn up on the checkout I have from clearleft, tweaked the > main.css for tabs/spaces and image paths, copied new images over. Not all of > these changes in the CSS impact the header/footer for this bug directly. > > Not sure what we should do going into the future to keep our stuff updated with > ongoing clearleft work. Do we get updates as to when it's safe to pull in > their work? Do we pull in updates as part of closing other bugs? > Let's keep it open to pick up the last of any of their changes. They've told us they will deliver the final product tomorrow for all pages. We chose to do it this way due to a delayed start and approaching timeline. You're doing a great job - thanks :)
Oh, one thing I noticed - you're deprecating a few strings in the .po file that shouldn't be.
Checked in the patch as r25715
(In reply to comment #26) > Oh, one thing I noticed - you're deprecating a few strings in the .po file that > shouldn't be. Not sure deprecating strings what that means - I ran the extract script and filled in copy for the new strings I added, but I didn't deprecate anything as far as I'm aware.
"Not sure what deprecating strings means" is what I meant to say - I think I need a localizer for my own bug comments.
It's the lines starting with #~ that you're adding to the .po file (and removing from the other part of the .po). That will happen if the script doesn't see the strings anymore, however, those strings still exist so it shouldn't happen.
Huh. I didn't do that by hand. I'm also working on the home page stuff for bug 490906, but didn't include it in this patch - I wonder if the strings have disappeared from there.
> > I also did an svn up on the checkout I have from clearleft, tweaked the > > main.css for tabs/spaces and image paths, copied new images over. Not all of > > these changes in the CSS impact the header/footer for this bug directly. > > > > Not sure what we should do going into the future to keep our stuff updated with > > ongoing clearleft work. Do we get updates as to when it's safe to pull in > > their work? Do we pull in updates as part of closing other bugs? > > > Let's keep it open to pick up the last of any of their changes. They've told > us they will deliver the final product tomorrow for all pages. > > We chose to do it this way due to a delayed start and approaching timeline. > You're doing a great job - thanks :) Hello, Just a note to say that it might be worth double checking the header markup from the clearleft repository, now that the tools dropdown and change application menus have been implemented. Also the footer code has changed this morning so it might be good to look at that too. The final templates will be with you on Wednesday of next week not today, please see the message on basecamp for further information on this. Kind regards, Natalie
Since basecamp is behind auth, here is Sophie's message: ------------------------ I wanted to keep you up to date with progress of the HTML/CSS. Natalie has been working through the templates this week and hopes to deliver the majority of them today. The Javascript functionality has taken longer to implement than expected and so there will be work further work that we need to complete early next week. We do still have two days in the budget and this should cover the remaining work. This should not hold up development. ------------------------ I take that to mean that only JS will be remaining after today.
Took a sweep through clearleft SVN diffs r2400:r2508, which I think goes back far enough to where I started. This patch is a merge by hand / tweaks by script of the shared stuff for header/footer layout. Will merge in homepage stuff for bug 490906. Adding r+ for clouserw and jbalogh for fun
Attachment #377730 - Flags: review?(jbalogh)
Attachment #377730 - Flags: review?(clouserw)
Attached file updated images
...and here's a tarball of images.
Never mind the patch, it's bunk thanks to commits out of order. Got an r(meh) from jbalogh in IRC, so I sorted out my check-ins and submitted r25797 and r25798
Attachment #377730 - Attachment is obsolete: true
Attachment #377730 - Flags: review?(jbalogh)
Attachment #377730 - Flags: review?(clouserw)
This isn't reflected in the mockups, but I think this is probably the best place to mention it: Almost all users will only have access to Developer Tools and not the other tools. So, if the user only has access to Developer Tools, we should just display that link instead of the dropdown menu in the header. The menu should be used if more than one tools link is available to that user.
From Natalie: ------------------------ Hello, As Sophie mentioned earlier we have been unable to provide you with a fully completed version of the code today. If you check subversion now however you should find the majority of the work. The only pages left to be completed are the: * Collection Extention page * Collection Extention first run page * Addon first run page (All the components for this are done but I need to work on the externals. I noticed this has no header and footer surrounding it, I assume its ok though to import existing stylesheets and do this as just a different type of page, different header and footer) We also want to do a bit more intensive browser testing and design QA at the beginning of next week, as well as finishing work on Add To Favourites. Can you please check the markup you have developed so far against our current version. Particularly the header and footer as these have changed a bit. Little else would have changed from the delivery last Friday for older things but I would advise checking. You will need to re-import all our images because I have compressed them now for performance and they are generally a lot smaller than they were. Naturally you will also have to re-import our CSS and Javascript Just a reminder, my php is for demonstrative purposes for the HTML only and not meant to be rolled into your build. We are now using a third party lightbox for the screenshots. Please see the pattern portfolio for usage (its just a case of adding rel=”lightbox” or rel=”lightbox-somenameforagroupofimages” on a link that links to a larger version of the screenshot). The documentation for this lighbox code is here: http://www.digitalia.be/software/slimbox2 There is 1.5 days left in the budget, we aim to get the remaining work in a final build for you on wednesday. It would be great to have a proper handover call with you guys after this, when would be good for you? Kind regards, Natalie ------------------------ I asked this morning if this means we're at a good point for us to merge again and they haven't replied.
hello, I am afraid I read your message about the merge after I left work this evening (we are in the UK). As I mentioned in my earlier mail, please do another merge of our code, the final version will be with you the middle of next week. I'll address the other issues when I get to work on Monday, Kind regards Natalie
<p id="brand"><a href="http://mozilla.com/">Mozilla</a></p> That is missing: * alt attribute * title attribute * www. in front of mozilla.com
(In reply to comment #40) > <p id="brand"><a href="http://mozilla.com/">Mozilla</a></p> > > That is missing: > > * alt attribute > * title attribute > * www. in front of mozilla.com Hmm. I can add the title and the www, but there's no image there to hang an alt attribute from.
Since this bug has morphed into tracking clearleft merges, I'll note that r26001 should have gotten us up to r2607 of the clearleft changes. That seems to be the latest as of this comment.
clouserw tells me that clearleft updates are done, so I'm closing this. File additional bugs if there are header/footer problems.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Verified FIXED; header/footer elements are in, and looking good. Will file new bugs when and if I notice them.
Status: RESOLVED → VERIFIED
(In reply to comment #41) > (In reply to comment #40) > > <p id="brand"><a href="http://mozilla.com/">Mozilla</a></p> > > > > That is missing: > > > > * alt attribute > > * title attribute > > * www. in front of mozilla.com > > Hmm. I can add the title and the www, but there's no image there to hang an > alt attribute from. just a quick note to say this doesnt need an alt attribute, the image is applied as a background, the alternative text is the 'Mozilla' that is inside the link. Screenreaders will read this out, it is positioned offscreen.
Quick heads-up: The new layout has a few flaws pointed out by th w3c validator that we should fix unless we have a reason not to. Some things are related to our added ARIA support and thus to be expected with "legacy" validators, others are fixable, such as <label for=...> pointing to nonexistent form fields in the advanced search interface.
(In reply to comment #46) > Quick heads-up: The new layout has a few flaws pointed out by th w3c validator > that we should fix unless we have a reason not to. > > Some things are related to our added ARIA support and thus to be expected with > "legacy" validators, others are fixable, such as <label for=...> pointing to > nonexistent form fields in the advanced search interface. stephend had raised this to me, but decided not to file a bug about it. If you think it's an issue, go ahead and file a bug and don't reopen this one. The comment thread's already massive enough
Attachment #377426 - Flags: review?(jbalogh)
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: