498825 - Rewrite install/download buttons so they don't have so many problems

Status: RESOLVED FIXED

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

Description

[Wil Clouser [:clouserw]]

We have a bunch of bugs with the install/download buttons and we've talked about rewriting the code for a long time.  It seems like we touch that code a lot and every time we do we collectively cringe and QA senses our concern and gets stressed out.

If we rewrite them with all the edge cases in mind we can make them easier to work on and more correct for the users.

Comment 1

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

+1

Comment 2

[Wil Clouser [:clouserw]]

<3

Comment 3

[Ryan Doherty (:rdoherty)]

::gulp::

Comment 4

[Wil Clouser [:clouserw]]

Attachment: omg

If you just got into the office you might want to grab some more coffee before you open this.

Comment 5

[Justin Scott [:fligtar]]

That is my new wallpaper.

Comment 6

[Ryan Doherty (:rdoherty)]

Attachment: Rough outline of plan

Here's a small patch to show the direction I'm going. Would like feedback earlier rather than later so I don't have to rewrite it twice ;)

Comment 7

[Wil Clouser [:clouserw]]

Comment on attachment 395968 [details] [diff] [review]
Rough outline of plan

I think the plan looks good

Comment 8

[Jeff Balogh (:jbalogh)]

Comment on attachment 395968 [details] [diff] [review]
Rough outline of plan

I wish you good luck sir.

Comment 9

[Justin Scott [:fligtar]]

This is P1 for 5.2 as it will block another 5.2 P1.

Comment 10

[Ryan Doherty (:rdoherty)]

Attachment: v1

PHP rewrite review commence!

JS rewrite is for next week.

Comment 11

[Wil Clouser [:clouserw]]

Comment on attachment 404390 [details] [diff] [review]
v1

I think it's looking good.  There are still some corner cases, but we can tweak it after the main classes have landed.

Also, it would be nice to fix this while you are creating the methods:

 $buttonMessage = sprintf(___('Add to %1$s %2$s'), APP_PRETTYNAME, "%s");

Comment 12

[Ryan Doherty (:rdoherty)]

r52887 

For QA:
Install buttons can be tested now, backend code was re-written. I'll put together a list of where they are displayed.

Comment 13

[Ryan Doherty (:rdoherty)]

(In reply to comment #11)
> Also, it would be nice to fix this while you are creating the methods:
> 
>  $buttonMessage = sprintf(___('Add to %1$s %2$s'), APP_PRETTYNAME, "%s");

This involves changes to the JavaScript too, I'll make it part of my next patch.

Comment 14

[Ryan Doherty (:rdoherty)]

URLs afaik (linking to preview for testing):

https://preview.addons.mozilla.org/en-US/firefox/browse/type:1/cat:22 (category landing)
https://preview.addons.mozilla.org/en-US/firefox/addon/3895 (add-on display)
https://preview.addons.mozilla.org/en-US/firefox/addons/policy/0/2032/57017 (policy page)
https://preview.addons.mozilla.org/en-US/firefox/pages/collector (collector info)
https://preview.addons.mozilla.org/en-US/firefox/addon/5579/developers (contributions)
https://preview.addons.mozilla.org/en-US/firefox/ (homepage)
https://preview.addons.mozilla.org/en-US/firefox/search?q=xmarks&cat=all (search)
https://preview.addons.mozilla.org/en-US/firefox/collection/webdeveloper (collection detail)

Comment 15

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

Ryan, so far, I think the changes are looking pretty good (haven't found anything obvious, anyway); how extensive is the JS rewrite, and what will it touch?

Thanks!

Comment 16

[Ryan Doherty (:rdoherty)]

The JS rewrite isn't as extensive as the PHP one, there is a lot less JS. But it will touch all button functions:

* Platform detection (FF too old/new)
* Experimental status (checkbox, etc)
* Thunderbird/sunbird install instructions
* Show/hide correct platform buttons

Rewrite will probably finish friday.

Comment 17

[Ryan Doherty (:rdoherty)]

Attachment: v2 (js)

Finally done (I hope)

Comment 18

[Wil Clouser [:clouserw]]

Comment on attachment 405815 [details] [diff] [review]
v2 (js)

works well for me!  Thanks.

Can you add L10n comments for any strings with substitutions in them?  I'd also like to see this die:

> + $buttonMessage = sprintf(___('Add to %1$s %2$s'), $appName, "%s");

but I realize it was grandfathered in.  We can do a separate bug if you don't get to it this time around.

Comment 19

[Ryan Doherty (:rdoherty)]

(In reply to comment #18)
> > + $buttonMessage = sprintf(___('Add to %1$s %2$s'), $appName, "%s");

Oh, yeah. I took a look at that and wasn't sure how to fix it initially. I'm guessing this would be better?

$buttonMessage = sprintf(___('Add to %1$s %%s'), $appName);

In JS, the string is then sprintf'd again to add the platform name.

Comment 20

[Wil Clouser [:clouserw]]

I think a switch/case. :-/

switch (appName):
  case 'firefox':
    sprintf(___(/* L10n: %s is a platform name. Example: Windows*/ 'Add to Firefox (%s)'), $platform);
    break;
...


Substituting the platform like that isn't the best, but I think it works.

Comment 21

[Ryan Doherty (:rdoherty)]

r53267

will spin off string fixes.