Closed Bug 431462 Opened 16 years ago Closed 16 years ago

generate download box with php code and put javascript out of in-product pages

Categories

(www.mozilla.org :: General, defect)

Product:
www.mozilla.org
Component:
General
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: pascalc, Assigned: clouserw)

References

Details

(Whiteboard: [server side])

Attachments

(3 files)

Patch with new download block function
8.28 KB, patch
Details | Diff | Splinter Review
patch to product details
12.24 KB, patch
reed
: review+
Details | Diff | Splinter Review
hardcode mozilla.com
2.40 KB, patch
reed
: review+
Details | Diff | Splinter Review 
Download boxes on in-product pages are full of javascript code just to generate a download box, a recent change to the markup inside of this javascript code means that I have to correct up to 150 pages just to add a <span> tag, and a search and replace won't work because localizers localized the strings inside the javascript code and some others didn't because they are not programmers and don't want to touch source code. Not to mention that some complained about that.

These boxes should be created with a php call and the 3 strings inside taken from main.lang where they are already translated for many locales. 

Steven, please create a php function that generates the box, make it generate the same javascript code if you wish (I don't see the point of locale detection in localized pages though, directly generating html makes more sense to me). It just is a waste of time to take hours to update manually tons of pages for a change to the javascript code.
Working on this today. Pascal, the code for this download box includes the following strings:

"Get Firefox 3"
"Download Firefox - Free" (the word "free" is in a span and styled separately in English)
"MacOS 9 and earlier are not supported"
"Free Download"
"%VERSION% for %PLATFORM_NAME%"
"%LANGUAGE_NAME%&nbsp;(%FILE_SIZE%)"
"Release Notes"
"Other Systems & Languages"

Should all of these strings go into the main.lang? Should we try to simplify them a bit?
VERSION, PLATFORM_NAME, LANGUAGE_NAME, FILE_SIZE are data we have from the productDetails php class so they shouldn't be in main.lang, I would replace the 'for' between version and platform_name because you can't know the gramatical rules for each language to associate the two values, it could be a word before the string or adding a suffic to platform_nmae for instance.

Release Notes -> main.lang
Other Systems & Languages -> main.lang
Free download  -> main.lang
Get Firefox 3 -> main.lang
MacOS 9 and earlier are not supported  -> main.lang
Download Firefox - Free -> main.lang, with the span

Several of these strings are already on main.lang on mozilla europe so I will just copy them over to mozilla.com for these locales.

My feeling is that the firefoxDetails class should have  a new method to generate the box or the class should be extended to add this method if we want to keep the class light. Lots of what is needed is already there.
Pascal - thanks for the further details.

Wil: How would you like this done? A new function similar to getDownloadBlockForLocale(), but with the javascripty stuff included? A sub-class of getDownloadBlockForLocale() for this particular arrangement of the download box?
Is there an example page for this?  The in-product pages I'm looking at (whatsnew and firstrun) don't have download buttons at all...

Also, I'll probably see after an example, but how is this different from getDownloadBlockForLocale()?  (An example of that is http://svn.mozilla.org/projects/mozilla.com/trunk/pt-BR/firefox/index.html - the "tweaks" line may or may not be necessary)
Release notes page has an example:

/en-US/firefox/3.0/releasenotes/

The just different in that it has some alternative layout and does the platform detection JS.
(In reply to comment #5)
> Release notes page has an example:
> 
> /en-US/firefox/3.0/releasenotes/
> 
> The just different in that it has some alternative layout and does the platform
> detection JS.
> 

On the en-US page, we should keep the js method (it could do with some cleanup though).  On the localized pages, we can use the getDownloadBlockForLocale() method.  Does that make sense?

(In reply to comment #6)
> On the en-US page, we should keep the js method (it could do with some cleanup
> though).  On the localized pages, we can use the getDownloadBlockForLocale()
> method.  Does that make sense?

That's ok with me. Pascal?
so, a function already exists.  Is this bug fixed?
Pascal, can you do this, or do you want one of us to?
what is the exact code to insert? I am not sure that the current html code we use for the download box is the same as on mozilla.com. Currently I added that and it appears unstyled:

<div id="sidebar">
	<script type="text/javascript" src="/js/download.js"></script>
	<?php echo $firefoxDetails->getDownloadBlockForLocale('fr', array('ancillary_links' => true)); ?>

I think that the method expects a parent div with a css class called main-feature but I don't see that on the code on the en-US version.

I tested on the french security page if you want to see it online.

Steven, have the big green download boxes on the home and firefox page and the small ones in the sidebar of subpages exactly the same html ? If not I think we need a second method for the class
Pascal, that's what Wil was proposing, I think. The sidebar download boxes are different HTML, yes.
if you need a different html,  we can't use this fonction.
I'm happy to add more functions or change the ones we have.  I just need to know what to add - Steven?

I might be able to figure it out if you guys can specify the pages better (example of sidebar of subpages?) but sgarrity could probably do it faster and more accurately.
Severity: normal → blocker
Severity: blocker → critical
Wil, we're basically looking for a function to output the HTML/JS as it appears on the current 3.0 Release Notes page:

https://www-firefox3.authstage.mozilla.com/en-US/firefox/3.0/releasenotes/

I could try it, but I'd be stumbling through some unfamiliar PHP. Can you set it up?
That code doesn't really work.  We made the original list a big <ul> with three <li>'s (one for each os) in it.

The code on that page does have an <li> in it, but it also has a <div> afterwards.  Can you change that code so each OS is it's own <li>?

Also, can the outer <ul> be consistent with what we have?  Our current front page uses:

<ul class="home-download">

and this code uses:

<ul class="download download-firefox os_$os">

Thanks.
Wil, not sure I understand the issue. The homepage doesn't seem to have the three <li> tags. Do we need that?
The front page on localized pages does.  If we want to use the productDetails library, we should use the three <li>'s.  (The en-US front page doesn't use the productDetails library)
Hey guys. I'm sure you're probably well aware of this, but I wanted to weigh in on the seriousness of this bug...it's basically blocking the Mozilla Europe localization being finished, so we need to get it resolved as soon as possible.

Steven and/or Will: can you update our PHP code to generate the same HTML that the Javascript Steven created generates?
Target Milestone: --- → 3.0
Wil, this is the same file I sent you via email. Wil this work? If so, I have a few CSS changes to commit as well.
It would work, but I'm not thrilled with all the duplication of code.  I'll attach a patch in a minute with another idea (which still isn't awesome).
Still makes for some ugly code, but I think it's a better format than the previous patch.  What do you think reed?

Designed to be used like this:

$_options = array( 'ancillary_links' => true, 'layout' => 'sidebar', 'download_parent_override' => 'sidebar');
echo $firefoxDetails->getDownloadBlockForLocale('en-US', $_options);
Attachment #321319 - Flags: review?(reed)
Hey guys. Just checking in on this...how close are we to having this resolved?
Attachment #321319 - Flags: review?(reed) → review?(morgamic)
Attachment #321317 - Attachment is patch: true
Attachment #321317 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 321319 [details] [diff] [review]
patch to product details

>             $_current_version = $this->getNewestVersionForLocale($locale);
...
>+                $_li_windows = $this->_getDownloadBlockListHtmlForLocaleAndPlatform($locale, 'Windows', $options);

What about passing $_current_version via _getDownloadBlockListHtmlForLocaleAndPlatform() so it doesn't have to be grabbed again?

>+         * @access protected
>+         * @param string locale (eg. de or en-US)
>+         * @param array optional parameters to adjust return value:

$platform is not documented.

>+                    $_download_title              = ___('Get Firefox 3');

Please don't hardcode the version, if at all possible.

>          * A convienence function for showing download links for multiple locales.

s/convienence/convenience/

With those fixes (or good reasons why not), r=reed.
Attachment #321319 - Flags: review?(morgamic) → review+
Also, r=morgamic with some nits about the fact that _li_* stuff could cause PHP notices/warnings in certain cases.
Thanks, most of the stuff was fixed.  I didn't add current_version to the call and I didn't mess with the notices because that's nothing new and we need to get this in.  sgarrity - can you verify this works for you? (r13445)
Assignee: nobody → clouserw
(In reply to comment #27)
> sgarrity - can you verify this works for you? (r13445)

Wil, can you give me an example of how to call the function with the Sidebar layout option?
(In reply to comment #28)
> (In reply to comment #27)
> > sgarrity - can you verify this works for you? (r13445)
> 
> Wil, can you give me an example of how to call the function with the Sidebar
> layout option?
> 

It's in comment #23
(In reply to comment #25)
> >+                    $_download_title              = ___('Get Firefox 3');
> 
> Please don't hardcode the version, if at all possible.

This issue didn't get addressed. We still host Thunderbird pages, so we need to be able to say "Get Thunderbird 2" or whatever for a while. You might need to add another override and/or use version info from other product-details stuff in order to display this text. We definitely don't need to restrict $_download_title to Firefox 3, as it _will_ cause problems in the future.
(In reply to comment #27)
> sgarrity - can you verify this works for you? (r13445)

Yes! This works great. I've got the related CSS committed in r13477.

(In reply to comment #25)
> Please don't hardcode the version, if at all possible.
Two suggestions:

a) we mangle the product name and version number into that type of string:

"Get {Product} {Version}"
(and we trim {Version} down to just the major release # before the '.'

b) setup a way to pass an arbitrary "title" string into the function
Also, pascal, can you confirm that this is working for you?
https://en-gb.www-firefox3.authstage.mozilla.com/en-GB/firefox/organic/

I put this code:
<div id="sidebar">

	<?php // change the locale code below by your code
	$_options = array( 'ancillary_links' => true, 'layout' => 'sidebar',
	'download_parent_override' => 'sidebar');
	echo $firefoxDetails->getDownloadBlockForLocale('en-GB', $_options);
	?>

	<ul class="what-is-mozilla">


The three boxes are showing up so I probably forgot to include some js file to hide them, Wil what should I add?

Also, this is for the sidebar, what is the code for the main download box on the home page?
(In reply to comment #33)
> The three boxes are showing up so I probably forgot to include some js file to
> hide them, Wil what should I add?

The os-detection js changes the CSS classes, and CSS then hides the other platforms. The CSS is included in r13477 (see lines #230 - #240).

> Also, this is for the sidebar, what is the code for the main download box on
> the home page?

The home page and Firefox page buttons aren't using the PHP functions. Wil, should they be?
>The os-detection js changes the CSS classes, and CSS then hides the other
platforms. The CSS is included in r13477 (see lines #230 - #240).

So why isn't it working ?


>The home page and Firefox page buttons aren't using the PHP functions. Wil,
should they be?

They do on mozilla europe and will on the brazilian version of mozilla.com, en-US using js is an exception, so yes, we need it.
> >The home page and Firefox page buttons aren't using the PHP functions. Wil,
> should they be?
> 
> They do on mozilla europe and will on the brazilian version of mozilla.com,
> en-US using js is an exception, so yes, we need it.
> 

As mentioned in comment #6, the en-US versions should continue to use the JS method, the other locales can use the PHP method.  For the other locales, you should be including download.js and then the same method as above:

<?php echo $firefoxDetails->getDownloadBlockForLocale('pt-BR'); ?> 

I'm not seeing the CSS affecting this either (still see all three blocks).
(In reply to comment #36)
> I'm not seeing the CSS affecting this either (still see all three blocks).

Where we talking about here?
This is working on the sidebar, but the html you generate for the big download box is totally different than what we have with :

<?php echo $firefoxDetails->getDownloadBlockForLocale('de'); ?> 

Which means that we don't have the green download box on mozilla europe, a major issue.
(In reply to comment #37)
> (In reply to comment #36)
> > I'm not seeing the CSS affecting this either (still see all three blocks).
> 
> Where we talking about here?
> 

The old structure we used to use.  Close to:

<script type="text/javascript" src="/js/download.js"></script>
<div id="main-feature">
    <div id="feature-contents">
      <?php echo $firefoxDetails->getDownloadBlockForLocale('de'); ?> 
    </div>
</div>
I can create the CSS for this to work with a green button like on the home page.
that would be great, thanks
the styling is working on mozilla europe home page but we have the 3 boxes instead of one and the ancilliary links have a huge font. On the firefox page, the styles are not applied at all. I also noticed that the surronding of the green box is not a real 24bits shadow in the alpha channel of the ong image but the sliced blue background. This is going to look bad for Norwegian because we display 2 boxes for them (2 languages) and the lower part of the bow will be on the white background of the page. Steven, can you fix these issues ? thanks
broken download boxes for all locales should be a blocker.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Pascal, is the mozilla europe development site somewhere I can see?
https://stage.mozilla-europe.org
login/pass is your lDAP authentification but I don't know if your account rights allow you to see it

What we really need and would be a time saver for us would be to have the php function to generate the same or a very similar HTML as you use on mozilla.com/en-US/ actually
I've change the download button to use a transparent-background PNG (with a hack for IE) in r13803.

The three download buttons should be down to one now as well (on the home page).
it looks good on the home page, both the background and the fact that we have the right number of boxes, but we still have the old background image on the firefox page

https://stage.mozilla-europe.org/no/
https://stage.mozilla-europe.org/no/firefox/
actually, the US firefox page has a much nicer layout than what we have on the european site, we don't even have the firefox logo in the box and there is actually no firefox logo at all on the page, which is not good.
(In reply to comment #45)
> What we really need and would be a time saver for us would be to have the php
> function to generate the same or a very similar HTML as you use on
> mozilla.com/en-US/ actually
> 

The surrounding html is the same for both types, the code for the inside changes a bit.  I'm online if anyone wants to chat about this today.
Whiteboard: [server side]
I've setup the CSS to be shared across the home/download and other pages with download buttons in r13911.

Pascal - does this work ok for you?
Whiteboard: [server side]
Whiteboard: [server side]
Forgot a file in my commit - r13914 adds it.
I talked to Pascal on IRC last night and I think this was working.  Pascal, can you confirm?  Are we done with this bug?
the page is working fine on mozilla europe but I haven't tested how it looks on in-product pages on mozilla.com and pt-BR should use it too so I'll leave this bug open a day or two, the time to check that everything is fine on all places where we use this code.
works now
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Woo hoo! This was an epic one...thanks to all of you for working on the fix.
reopening, I need the ancillary links to be absolute and not relative, otherwise they won't work on mozilla europe :

<p class="download-other">
<a class="ancillaryLink" href="/fr/firefox/2.0.0.14/releasenotes/">Notes de version</a> -
<a class="ancillaryLink" href="/fr/firefox/all.html">Autres langues et systèmes</a>
</p>


Should be:

<p class="download-other"><a class="ancillaryLink" href="http://fr.www.mozilla.com/fr/firefox/2.0.0.14/releasenotes/">Notes de version</a> -
<a class="ancillaryLink" href="http://fr.www.mozilla.com/fr/firefox/all.html">Autres langues et systèmes</a>
</p>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think I wanted these to be more generic originally but they weren't used elsewhere, it's an easy fix now, and it should avoid any 404s and weird redirects.
Attachment #323631 - Flags: review?(reed)
Comment on attachment 323631 [details] [diff] [review]
hardcode mozilla.com

*cry*
Attachment #323631 - Flags: review?(reed) → review+
r14608
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Component: www.mozilla.org/firefox → www.mozilla.org
Component: www.mozilla.org → General
Product: Websites → www.mozilla.org
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: