Feature #11722

Review JavaScript on new donation page

Added by sajolida 2016-08-25 10:46:19 . Updated 2016-09-22 05:23:15 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Fundraising
Target version:
Start date:
2016-08-25
Due date:
% Done:

10%

Feature Branch:
web/10176-redesign-donate
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

The new donation page uses JavaScript to prevent a single interface that allows switching between dollars and euros and populates a PayPal form.

I wrote it using JQuery to make it easy for me to have a prototype but someone else who know what she’s doing should have a look and write real code in there.

Let’s do that in the web/10176-redesign-donate but let’s focus on the JavaScript code only in this ticket. The rest can go in Feature #10176.


Files

jquery.js (268380 B) sajolida, 2016-09-04 08:26:26
button.js (3815 B) sajolida, 2016-09-04 08:26:27

Subtasks


History

#1 Updated by sajolida 2016-08-25 10:55:45

  • Assignee deleted (None)
  • Target version set to Tails_2.6

u: do you feel like doing this? If so under which deadline?

The page should ideally be ready a bit before the campaign so we can debug it for some days before starting the run. That would be around 2.6, does that seem feasible to you? Otherwise maybe I can ask other people? volenta, co6?

Maybe it would be good to:

  • Keep the selected amount when switching currency. For example, if I select $20 (or type in $42 as custom amount) and then switch to euros, I should still have $20 (or $42) selected.
  • Would it be better to populate “currency”, “business”, and “currency_code” programmatically on start instead of in the HTML?

#2 Updated by sajolida 2016-08-25 11:01:40

  • Feature Branch set to web/10176-redesign-donate

#3 Updated by sajolida 2016-09-04 08:26:30

Also, JQuery and Bootstrap buttons are needed for this page to work but I didn’t put them in Git. You’ll see the /lib/js on top of donate.html. I’m attaching here the versions I used. You should put them in /lib/js/jquery.js and /lib/js/bootstrap/button.js@ respectively.

#4 Updated by Anonymous 2016-09-17 14:49:56

  • Assignee set to sajolida
  • % Done changed from 0 to 10
  • Feature Branch changed from web/10176-redesign-donate to 451f:web/10176-redesign-donate

TECHNICAL ISSUES
- I could not get the scripts to load when I built the wiki and had to manually edit the file to get it to run. Could you verify that this works correctly for you? Do you have an idea why this did not work for me?

RECOMMENDATIONS

- I would not use the whole Bootstrap CSS. This makes the website so heavy. Why not just copy the relevant parts from this CSS to ours?

- The jQuery js library should be used in its minified version. You wrote in the HTML of the donate page a comment, where you say that you used 1.9.1.min. However the version you uploaded on the parent ticket was not the minified one.

- It would be nice that when we click on Donate, this opens a new page. (done)

- We can very much simplify this script if we use classes instead of IDs and then we can toggle .euros/.dollars with a one-liner. (done)

- Radio inputs can and should have values. We can use these values in the script. And write only one function for all of the buttons. (done)

- Whenever you close a function, put a semicolon after “});” otherwise there might be errors with some Javascript interpreters. (done)

- You’ve sort of mixed jQuery and Vanilla JS. For example, document.getElementById is Vanilla JS. In jQuery, one would simply write $(‘#myelement’). I did not change that yet for every line, because I think you should do that yourself. It doesn’t really matter, because the browser interprets it in any case. The jQuery way is easier to write I believe.

- On the thanks page, I’d suggest to add a target=“_blank” to the links with class “tweet”. Please note that target=“_blank” is perfectly fine in HTML5. (http://stackoverflow.com/questions/4198788/is-it-alright-to-use-target-blank-in-html5 and corresponding W3C drafts.)
- I’d like to try to get rid of button.js. I think that this script does not do much which justifies an inclusion in the website. Especially, if we load jQuery anyway, we can just write this code ourselves.

QUESTIONS

- What’s the idea of modifying the return-url value via JS? I think it’s completely useless and could be dropped.

- It’s currently unclear to me if we really need to use jQuery or not at all. But if we actually don’t want to use it, I need more time to convert what has been done to Vanilla JS.
- I dont see why we have twice the same content (paypal-with-js, paypal-without-js). This should simply be a graceful degradation. When the user disabled JS, she should simply be able to use the form without the visual enhancement. But I’m a bit short with time and thus won’t fix that myself - at least not for this donation campaign.

I’ve pushed some changes to my repository, using the same branch name. I’ll try to push some more modifications in order to get rid of button.js and then I might know if I feel capable of remodifying all the scripts to get rid of jQuery too.
Don’t hesitate to get back to me in the meantime about anything related to this.

UPDATE: I’ve gotten rid of button.js.

#5 Updated by Anonymous 2016-09-17 14:50:04

  • QA Check set to Dev Needed

#6 Updated by Anonymous 2016-09-17 15:59:33

Oh, and we should add Javascript on the bottom of the page, not in the head so that the page will load the contens first and then the JS files.

#7 Updated by sajolida 2016-09-19 13:01:22

  • Target version changed from Tails_2.6 to Tails_2.7

#8 Updated by sajolida 2016-09-19 14:52:47

> - I could not get the scripts to load when I built the wiki and had to manually edit the file to get it to run. Could you verify that this works correctly for you? Do you have an idea why this did not work for me?

I need more info. What’s the resulting HTML and what’s wrong with it?
Which scripts are missing? All? Which ikiwiki configuration are you
using? For example, make sure you have ‘donate’ and its friends in
htmlscrubber_skip, see 515f9a0.

> - I would not use the whole Bootstrap CSS. This makes the website so heavy. Why not just copy the relevant parts from this CSS to ours?

As a reminder: we’re already using Bootstrap on the Installation
Assistant. Tchou refused to work without a CSS framework for this, and
after seeing it in action I totally understand his point. It’s sooo much
easier and faster to build well-structured pages and responsive layout
that I’m not ready to go back to pure CSS for anything slightly
elaborate (like this page). Now, I’m a newbie here and surely have tons
of things to learn. For example there might be other such frameworks but
smaller or ways of extracting only the relevant CSS bits once your
design is ready. If you know about such things, I’ll need your help
because I don’t know how to do this myself.

Regarding the heaviness, I quickly compared one of our pages with
Bootstrap and a few images
(https://tails.boum.org/install/debian/usb/overview/) and 91 KB of
minified jQuery with other donation pages that I used as sources of
inspiration. I used the developers tools from Tor Browser, in the
Network tab:

488 KB: https://tails.boum.org/install/debian/usb/overview/

585 KB:
https://donate.wikimedia.org/w/index.php?title=Special:FundraiserLandingPage&country=XX&uselang=en&utm_medium=sidebar&utm_source=donate&utm_campaign=C13_en.wikipedia.org

751 KB: https://www.torproject.org/donate/donate.html.en

1793 KB:
https://donate.libreoffice.org/home/dl/win-x86/5.1.3/en-US/LibreOffice_5.1.3_Win_x86.msi

2035 KB: https://supporters.eff.org/donate/button

397 KB looks like a lot to me in absolute, but compared with what others
do it’s not that crazy. I’m not saying we should be heavy because others
are heavy but it seems like we’re not that bad. On our page most of the
weight is on:

- jquery.min.js: 91 KB (against 263K for the non-minified version)

- bootstrap.css: 138 KB
- SourceSansPro-Regular.ttf: 146 KB

Also our version of Bootstrap is not minified. A minified version goes
down from 143 KB to 119 KB. Would it be worth it? Any downsides?

I personally don’t want to spend much time changing the foundations of
the CSS behind this page, but I’d be happy to learn quick tips to
optimize the weight of our pages in general (or be proposed an
alternative to Bootstrap).

> - The jQuery js library should be used in its minified version.

Yes, if we keep jQuery.

> - It would be nice that when we click on Donate, this opens a new page. (done)

You mean using a target=“_blank”? Why? We have one on
/contribute/how/donate but I removed it because I thought donating was a
linear process 1. /donate 2. PayPal 3. /donate/thanks and thus that it
was making sense to have it happen in a single tab. It also doesn’t make
much sense for people to keep /donate open in a different tab since they
will be anyway forwarded back to our website after the PayPal form (or
using the back button), and /donate is like a dead end with basically no
links to the outside. But now, I might not be aware of all the best
practices regarding this…

> - We can very much simplify this script if we use classes instead of IDs and then we can toggle .euros/.dollars with a one-liner. (done)

Smart!

> - Radio inputs can and should have values. We can use these values in the script. And write only one function for all of the buttons. (done)

Nice!

> - Whenever you close a function, put a semicolon after “});” otherwise there might be errors with some Javascript interpreters. (done)

Ok, I’ll try to remember…

> - You’ve sort of mixed jQuery and Vanilla JS. For example, document.getElementById is Vanilla JS. In jQuery, one would simply write $(‘#myelement’). I did not change that yet for every line, because I think you should do that yourself. It doesn’t really matter, because the browser interprets it in any case. The jQuery way is easier to write I believe.

Like in 037ea47?

> - On the thanks page, I’d suggest to add a target=“_blank” to the links with class “tweet”. Please note that target=“_blank” is perfectly fine in HTML5. (http://stackoverflow.com/questions/4198788/is-it-alright-to-use-target-blank-in-html5 and corresponding W3C drafts.)

This I like! Done in 04c4375.

> - I’d like to try to get rid of button.js. I think that this script does not do much which justifies an inclusion in the website. Especialy, if we load jQuery anyway, we can just write this code ourselves.

Yes! I didn’t see you code yet but that’s a great idea.

> - What’s the idea of modifying the return-url value?

I put anchors in every place where we have a link to the donation page.
For example, each tweet template points to a different anchor on the
donation page and the link that we will add at the bottom of tails-bugs
emails will also have a different one. The idea is to be able to do
stats on where people are coming from to donate (which fundraising
efforts bring in more money). Then I thought that we could also do stats
on which of these anchors lead to more donations (“= conversion rate”),
and we could do this by forwarding the anchor to the thanks page and do
stats on the thanks page. I thought that my comment on lines 3 and 4 of
donation.js would explain this but maybe I failed…

> - It’s currently unclear to me if we really need to use jQuery or not at all. But if we actually don’t want to use it, I need more time to convert what has been done to Vanilla JS.

Right, that’s what I understood as well. I wrote jQuery because
basically it’s the only kind of JS that I can write without turning
crazy. But I’d be happy to have the code rewritten in Vanilla JS.
Still, should I include a minified version of jQuery or do you want to
work on this before the first merge?

> - I dont see why we have twice the same content (paypal-with-js, paypal-without-js). This should simply be a graceful degradation. When the user disabled JS, she should simply be able to use the form without the visual enhancement. But I’m a bit short with time and thus won’t fix that myself.

If that’s possible, yes. Here the JavaScript and the degraded versions
have quite different layouts (see for example the way “recurring” and
“one-time” donations are handled very differently in the structure of
the page). That’s why I thought it would be much easier like this.

> I’ve pushed some changes to my repository, using the same branch name.

I merged your branch and pushed it to origin.

> I’ll try to push some more modifications in order to get rid of
button.js and then I might know if I feel capable of remodifying all the
scripts to get rid of jQuery too.

Ok, let’s see if we can get rid of jQuery!

> Don’t hesitate to get back to me in the meantime about anything related to this.

Thanks a lot! It’s cool to write and review code sometimes!

Basically now we have to discuss a bit more about target=“blank”,
whether or not replacing jQuery before the merge, and the Bootstrap
thing. So we’re almost there, that’s so cool!

#9 Updated by sajolida 2016-09-19 14:59:18

> Oh, and we should add Javascript on the bottom of the page, not in the head so that the page will load the contens first and then the JS files.

Having a very quick look, it seems like our meta tags for scripts are
added by <TMPL_IF META><TMPL_VAR META></TMPL_IF> in template/page.tmpl
and so this would be a single block, together with others meta tags
(like the additional stylesheets). I suppose that putting stylesheets
are the botton is a bad practice (but I don’t know).

So I’m not sure how we could do this in ikiwiki (in a way that’s worth
it)…

#10 Updated by sajolida 2016-09-19 14:59:47

  • Assignee deleted (sajolida)
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch changed from 451f:web/10176-redesign-donate to web/10176-redesign-donate

#11 Updated by intrigeri 2016-09-20 03:10:40

>> Oh, and we should add Javascript on the bottom of the page, not in the head so that the page will load the contens first and then the JS files.

> So I’m not sure how we could do this in ikiwiki (in a way that’s worth it)…

commit:ef80d23 might help.

#12 Updated by Anonymous 2016-09-20 15:39:00

  • Assignee set to sajolida
  • QA Check changed from Ready for QA to Dev Needed

sajolida wrote:
> > - I could not get the scripts to load when I built the wiki and had to manually edit the file to get it to run. Could you verify that this works correctly for you? Do you have an idea why this did not work for me?
>
> I need more info. What’s the resulting HTML and what’s wrong with it?
> Which scripts are missing? All? Which ikiwiki configuration are you
> using? For example, make sure you have ‘donate’ and its friends in
> htmlscrubber_skip, see 515f9a0.

I used what you’ve pushed to Git, added the libraries in the correct place and then I got some message about META script not found, or something like that. I did not verify the htmlscrubber thing, but if it’s just that, let’s not bother.

> > - I would not use the whole Bootstrap CSS. This makes the website so heavy. Why not just copy the relevant parts from this CSS to ours?
>
> As a reminder: we’re already using Bootstrap on the Installation
> Assistant. Tchou refused to work without a CSS framework for this, and
> after seeing it in action I totally understand his point. It’s sooo much
> easier and faster to build well-structured pages and responsive layout
> that I’m not ready to go back to pure CSS for anything slightly
> elaborate (like this page). Now, I’m a newbie here and surely have tons
> of things to learn. For example there might be other such frameworks but
> smaller or ways of extracting only the relevant CSS bits once your
> design is ready. If you know about such things, I’ll need your help
> because I don’t know how to do this myself.

I’m fine with Bootstrap but often, like on the Donate page, you just want some definitions for the columns and the buttons and thus, I personally would just copy these definitions and that’s it. You can customize all Bootstrap downloads here: http://getbootstrap.com/customize/ if you think it’s a good idea. Otherwise let’s not bother.

> Regarding the heaviness, I quickly compared one of our pages with
> Bootstrap and a few images
> (https://tails.boum.org/install/debian/usb/overview/) and 91 KB of
> minified jQuery with other donation pages that I used as sources of
> inspiration. I used the developers tools from Tor Browser, in the
> Network tab:

[..]

> 397 KB looks like a lot to me in absolute, but compared with what others
> do it’s not that crazy. I’m not saying we should be heavy because others
> are heavy but it seems like we’re not that bad. On our page most of the
> weight is on:
>
> - jquery.min.js: 91 KB (against 263K for the non-minified version)
> - bootstrap.css: 138 KB
> - SourceSansPro-Regular.ttf: 146 KB
>
> Also our version of Bootstrap is not minified. A minified version goes
> down from 143 KB to 119 KB. Would it be worth it? Any downsides?

The downside is that we could not modify this easily in Git, as it’s gonna be one single line. But as this is a framework, we should not modify it. So It’s ok to use a minified version of bootstrap.css.

> I personally don’t want to spend much time changing the foundations of
> the CSS behind this page, but I’d be happy to learn quick tips to
> optimize the weight of our pages in general (or be proposed an
> alternative to Bootstrap).

I don’t have an alternative. But we could start using Less or Sass at some point in the future :)

> > - The jQuery js library should be used in its minified version.
>
> Yes, if we keep jQuery.

ack.

> > - It would be nice that when we click on Donate, this opens a new page. (done)
>
> You mean using a target=“_blank”? Why? We have one on
> /contribute/how/donate but I removed it because I thought donating was a
> linear process 1. /donate 2. PayPal 3. /donate/thanks and thus that it
> was making sense to have it happen in a single tab. It also doesn’t make
> much sense for people to keep /donate open in a different tab since they
> will be anyway forwarded back to our website after the PayPal form (or
> using the back button), and /donate is like a dead end with basically no
> links to the outside. But now, I might not be aware of all the best
> practices regarding this…

Ack, it’s a linear process. You’re probably right about this one because you’re supposed to be sent back somewhere.

> > - On the thanks page, I’d suggest to add a target=“_blank” to the links with class “tweet”. Please note that target=“_blank” is perfectly fine in HTML5. (http://stackoverflow.com/questions/4198788/is-it-alright-to-use-target-blank-in-html5 and corresponding W3C drafts.)
>
> This I like! Done in 04c4375.

cool!

> > - I’d like to try to get rid of button.js. I think that this script does not do much which justifies an inclusion in the website. Especialy, if we load jQuery anyway, we can just write this code ourselves.
>
> Yes! I didn’t see you code yet but that’s a great idea.

My code works without it already, so we can simply delete the call to that file.

> > - What’s the idea of modifying the return-url value?
>
> I put anchors in every place where we have a link to the donation page.
> For example, each tweet template points to a different anchor on the
> donation page and the link that we will add at the bottom of tails-bugs
> emails will also have a different one. The idea is to be able to do
> stats on where people are coming from to donate (which fundraising
> efforts bring in more money). Then I thought that we could also do stats
> on which of these anchors lead to more donations (“= conversion rate”),
> and we could do this by forwarding the anchor to the thanks page and do
> stats on the thanks page. I thought that my comment on lines 3 and 4 of
> donation.js would explain this but maybe I failed…

Ok, I see. I read the comment but I tried for example to cancel a donation and i got sent back to a URL without hash so i was not sure what this was supposed to be. Ok, so I need to check that this works.

> > - It’s currently unclear to me if we really need to use jQuery or not at all. But if we actually don’t want to use it, I need more time to convert what has been done to Vanilla JS.
>
> Right, that’s what I understood as well. I wrote jQuery because
> basically it’s the only kind of JS that I can write without turning
> crazy. But I’d be happy to have the code rewritten in Vanilla JS.
> Still, should I include a minified version of jQuery or do you want to
> work on this before the first merge?

Please include a minified jQuery first maybe. I’ll try to convert this to vanilla JS though I think.

> > - I dont see why we have twice the same content (paypal-with-js, paypal-without-js). This should simply be a graceful degradation. When the user disabled JS, she should simply be able to use the form without the visual enhancement. But I’m a bit short with time and thus won’t fix that myself.
>
> If that’s possible, yes. Here the JavaScript and the degraded versions
> have quite different layouts (see for example the way “recurring” and
> “one-time” donations are handled very differently in the structure of
> the page). That’s why I thought it would be much easier like this.

Works for me. I think it could have been done in a graceful degradation way too but let’s not bother.

> I merged your branch and pushed it to origin.

cool.

> Ok, let’s see if we can get rid of jQuery!

yep.

> Thanks a lot! It’s cool to write and review code sometimes!

yay!

> Basically now we have to discuss a bit more about target=“blank”,
> whether or not replacing jQuery before the merge, and the Bootstrap
> thing. So we’re almost there, that’s so cool!

I think I just answered all that.

#13 Updated by Anonymous 2016-09-20 15:39:36

intrigeri wrote:
> >> Oh, and we should add Javascript on the bottom of the page, not in the head so that the page will load the contens first and then the JS files.
>
> > So I’m not sure how we could do this in ikiwiki (in a way that’s worth it)…
>
> commit:ef80d23 might help.

So simply adding it using