Opened 3 years ago

Closed 3 years ago

#20414 closed defect (fixed)

Donation banner on about:tor page for 2016 campaign

Reported by: arthuredelstein Owned by:
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201611R, crowdfunding
Cc: brade, mcs, gk, ssteele, isabela Actual Points:
Parent ID: #20413 Points:
Reviewer: Sponsor:

Description

Like last year (#17565), we want to create a donation banner on about:tor. Perhaps parts of the old code can be reused.

Child Tickets

Attachments (8)

banner1.png (89.5 KB) - added by arthuredelstein 3 years ago.
banner2.png (92.4 KB) - added by arthuredelstein 3 years ago.
banner3.png (92.5 KB) - added by arthuredelstein 3 years ago.
banner4.png (86.7 KB) - added by arthuredelstein 3 years ago.
banner1a.png (83.1 KB) - added by arthuredelstein 3 years ago.
banner2a.png (80.8 KB) - added by arthuredelstein 3 years ago.
banner3a.png (85.2 KB) - added by arthuredelstein 3 years ago.
banner4a.png (78.6 KB) - added by arthuredelstein 3 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 3 years ago by arthuredelstein

Parent ID: #20413

comment:2 Changed 3 years ago by mcs

Cc: brade mcs added

comment:3 Changed 3 years ago by arthuredelstein

Cc: gk ssteele isabela added
Keywords: TorBrowserTeam201610R crowdfunding added
Status: newneeds_review

I'm working on the banner, and it's getting there, but it still needs some polishing. In the meantime, here are the strings (for review) we are going to use for the banner text. (This text was suggested by Shari, with minor revisions.) If these strings look OK, I would like to get these added to the repository ASAP so we can give the translators a chance to do the translation ahead of our deadline for the next release.

https://github.com/arthuredelstein/torbutton/commit/20414+2

(For reference, I will post screenshots of the banner in the next comment.)

Changed 3 years ago by arthuredelstein

Attachment: banner1.png added

Changed 3 years ago by arthuredelstein

Attachment: banner2.png added

Changed 3 years ago by arthuredelstein

Attachment: banner3.png added

Changed 3 years ago by arthuredelstein

Attachment: banner4.png added

comment:4 Changed 3 years ago by arthuredelstein

(These four variants of the banner are rotated randomly.)





Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:5 in reply to:  3 ; Changed 3 years ago by mcs

Replying to arthuredelstein:

I'm working on the banner, and it's getting there, but it still needs some polishing. In the meantime, here are the strings (for review) we are going to use for the banner text. (This text was suggested by Shari, with minor revisions.) If these strings look OK, I would like to get these added to the repository ASAP so we can give the translators a chance to do the translation ahead of our deadline for the next release.

https://github.com/arthuredelstein/torbutton/commit/20414+2

The strings look okay to me. I am old school, so I would capitalize Internet... but I suspect that is not something the cool kids do these days.

comment:6 in reply to:  5 Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to arthuredelstein:

I'm working on the banner, and it's getting there, but it still needs some polishing. In the meantime, here are the strings (for review) we are going to use for the banner text. (This text was suggested by Shari, with minor revisions.) If these strings look OK, I would like to get these added to the repository ASAP so we can give the translators a chance to do the translation ahead of our deadline for the next release.

https://github.com/arthuredelstein/torbutton/commit/20414+2

The strings look okay to me. I am old school, so I would capitalize Internet... but I suspect that is not something the cool kids do these days.

At the risk of losing my Cool Kid status, I think I prefer it capitalized as well. Here's my new version:

https://github.com/arthuredelstein/torbutton/commit/20414+4

comment:7 Changed 3 years ago by arthuredelstein

Here's the HTML/JS/CSS patch for review. For now it's only visible on US-English locales, but when we the strings have been translated, we can add another patch to show more locales.

https://github.com/arthuredelstein/torbutton/commits/20414+5
(two patches)

Changed 3 years ago by arthuredelstein

Attachment: banner1a.png added

Changed 3 years ago by arthuredelstein

Attachment: banner2a.png added

Changed 3 years ago by arthuredelstein

Attachment: banner3a.png added

Changed 3 years ago by arthuredelstein

Attachment: banner4a.png added

comment:8 Changed 3 years ago by arthuredelstein

After feedback from Shari, I've modified the text and layout somewhat. Please see images below.

The new patches are here:
https://github.com/arthuredelstein/torbutton/commits/20414+8
The patch for strings alone is at
https://github.com/arthuredelstein/torbutton/commit/9eed15d158f4763471150a97c83bb8d772774d57





comment:9 Changed 3 years ago by gk

I cherry-picked the strings (commit 269031f7aba449312189f73753e616fb82e5af48).

comment:10 Changed 3 years ago by mcs

Kathy and I reviewed the JS/HTML/CSS changes. They look okay; we noticed just a few small things:

  • The comment says "2016 Dec 1" for the start date but the code uses October 1st.
  • Please remove some of the console.log() statements.
  • "fitt" should be "fit" in one comment.
  • The CSS has some rules for other browsers, e.g., -khtml-user-select: none (but if the same CSS file is going to be used on the website, maybe you do not want to remove these).
  • It would be nice to clean up the extensions.torbutton.donation_banner2016.shown_count pref somehow. Maybe reset it after the end date has been reached? This is not a big deal though; I do not know if we cleaned up last year's pref.

comment:11 in reply to:  10 Changed 3 years ago by arthuredelstein

Replying to mcs:

Kathy and I reviewed the JS/HTML/CSS changes. They look okay; we noticed just a few small things:

Thanks for the review and catching all my mistakes!

  • The comment says "2016 Dec 1" for the start date but the code uses October 1st.

Fixed. The comment and code both now set a start date of November 15.

  • Please remove some of the console.log() statements.

Done.

  • "fitt" should be "fit" in one comment.

Fixed.

  • The CSS has some rules for other browsers, e.g., -khtml-user-select: none (but if the same CSS file is going to be used on the website, maybe you do not want to remove these).

Yes, I'll leave those in for that reason.

  • It would be nice to clean up the extensions.torbutton.donation_banner2016.shown_count pref somehow. Maybe reset it after the end date has been reached? This is not a big deal though; I do not know if we cleaned up last year's pref.

I've added some code to clean up the pref.

New version: https://github.com/arthuredelstein/torbutton/commit/20414+9

comment:12 Changed 3 years ago by gk

Keywords: TorBrowserTeam201611 added; TorBrowserTeam201610R removed
Status: needs_reviewneeds_revision

okay, I gave it a whirl and it looks good to me with some nits addressed:

1) What is the scope of count? It seems to me we could rewrite

    if (Services.prefs.prefHasUserValue(shownCountPref)) {
      count = Services.prefs.getIntPref(shownCountPref);
    } else {
      count = 0;
    }

into

    let count = 0;
    if (Services.prefs.prefHasUserValue(shownCountPref)) {
      count = Services.prefs.getIntPref(shownCountPref);
    }

2) s/that stop observing/that stops observing/

3) We put in the translations early to give translators time to produce the properly localized strings for non en-US Tor Browser versions. Yet the code is only concerned with en-US bundles anyway. What is the plan here then? Don't we want to ship 6.0.6 with the donation banner enabled for as many users as possible (this is more a needs_information point as I am not sure whether we should fix the code or not)?

comment:13 in reply to:  12 Changed 3 years ago by arthuredelstein

Replying to gk:

okay, I gave it a whirl and it looks good to me with some nits addressed:

Thanks for the review.

1) What is the scope of count?

Fixed.

2) s/that stop observing/that stops observing/

Fixed.

3) We put in the translations early to give translators time to produce the properly localized strings for non en-US Tor Browser versions. Yet the code is only concerned with en-US bundles anyway. What is the plan here then? Don't we want to ship 6.0.6 with the donation banner enabled for as many users as possible (this is more a needs_information point as I am not sure whether we should fix the code or not)?

I have currently added several locales. I hope more can be translated in time for the release (including French and Russian). If so, we'll need to add those to the kBannerLocales list when they are ready.

New version: https://github.com/arthuredelstein/torbutton/commit/20414+11

comment:14 Changed 3 years ago by arthuredelstein

Status: needs_revisionneeds_review

comment:15 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

I applied .2 to master and maint-1.9.5 (commits c8ec31a111cf15971a341df02bca04fe09dd3bed and f5b70b5a4d5f5fd8db9acb74d62c3cc1b86f3ee2).

After updating the translations on master I realized that tr is ready as well and activates that one on maint-1.9.5 (6af7c834e9f79af974625d0f925d064a098bbee8) after getting the banner translations on that branch as well (94e5cfa96ed3ce4a3172efcbda8638e280de637e).

comment:16 Changed 3 years ago by gk

Keywords: TorBrowserTeam201611R added; TorBrowserTeam201611 removed

(I guess it can't hurt to test the code better before 6.0.6 is getting out, though ;) )

comment:17 Changed 3 years ago by arthuredelstein

Resolution: fixed
Status: closedreopened

I copied Isabela's pt-BR translation of the banner strings over to the pt locale in Transifex; they are now available for import.

Here's another patch that adds the pt locale, delays launch by a week to November 22 (because the donation page needs a little more preparation time), and improves the font-size and appearance of the donate button.

https://github.com/arthuredelstein/torbutton/commit/20414+12

comment:18 Changed 3 years ago by gk

Resolution: fixed
Status: reopenedclosed

Alright. Looks good to me. Applied to master and maint-1.9.5 (commit 64f16026a2e9899c136e925f4487721d2746c56e and ba95ab383ce6ecdcdba9cc905095b0a85ad1b08e). I updated the pt translations on maint-1.9.5 with commit e6ce4a86360b4b17f690dcd229bb8b478fa19679.

comment:19 Changed 3 years ago by arthuredelstein

Resolution: fixed
Status: closedreopened

Here's one more minor revision. If there isn't time to include this revision in the release, it's not really a problem. This patch simply delays the banner launch by one more day (to November 23) and also fixes a minor mistake in the code that doesn't stop it from functioning correctly.

https://github.com/arthuredelstein/torbutton/commit/20414+13

comment:20 Changed 3 years ago by arthuredelstein

Status: reopenedneeds_review

comment:21 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Fixed with 2f8570e04f2465710b02abaf1e0cfd3059580134 (master) and 1a79b90c4cf41558d2b637b6314e0c64338d158d (maint-1.9.5).

comment:22 Changed 3 years ago by arthuredelstein

Resolution: fixed
Status: closedreopened

I just discovered that the right arrow on the donate button that I just added is failing to render on OS X. Very sorry about this last minute discovery! Here's a patch to fix it:

https://github.com/arthuredelstein/torbutton/commit/20414+14

(I have tested this latest patch on all 3 platforms and it looks OK.)

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:23 Changed 3 years ago by arthuredelstein

Status: reopenedneeds_review

comment:24 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Applied to master and maint-1.9.5 (0a5af31aed37e4fd9faec408455b2d4ae139841e and e22642323f09efd18b4315d0b7145a781bb3dbdc). It won't make it into 6.5a4. We try to get it squeezed into 6.0.6, though.

Note: See TracTickets for help on using tickets.