Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Improves the donation handling #1731
Conversation
kuzzmi
added some commits
Aug 5, 2017
kuzzmi
changed the title from
Feature donation
to
Improves the donation handling
Aug 5, 2017
|
This comment may be out of the scope of this particular effort (particularly because we currently don't have any dynamic content capabilities deployed) but it would be good to eventually have a unique payment address (perhaps BIP32-based) for each user for privacy reasons. |
|
@crwatkins It makes sense, however, the transparency is equally important in my opinion. Anyway, with current configuration it won't be difficult to tweak the front-end code to support this. |
Francisc
reviewed
Aug 5, 2017
Left a few comments. Please let me know if there's anything not clear enough.
| @@ -16,6 +16,8 @@ | ||
| {% if page.lang == 'bg' or page.lang == 'el' or page.lang == 'ko' or page.lang == 'hi' or page.lang == 'pl' or page.lang == 'sl' or page.lang == 'ro' or page.lang == 'ru' or page.lang == 'tr' or page.lang == 'uk' or page.lang == 'zh_CN' or page.lang == 'zh_TW' %}<link rel="stylesheet" href="/css/sans.css">{% endif %} | ||
| <script type="text/javascript" src="/js/base.js"></script> | ||
| {% if page.id != 'download' %}<script type="text/javascript" src="/js/main.js"></script>{% endif %} | ||
| +<script src="/js/jquery/jquery-1.11.2.min.js"></script> |
Francisc
Aug 5, 2017
jQuery is way too expensive to be added as a dependency for the banner alone.
As far as I can see, it's only loaded on a handful of pages.
kuzzmi
Aug 6, 2017
Contributor
It's used for QR code generation. Currently nginx uses very long-living cache and Gzip, so I don't see a big problem in the long run, as in the end it will result in additional 32kb file to be loaded.
If this is everybody's concern, I'll rework this without jQuery.
Cobra-Bitcoin
Aug 6, 2017
Contributor
jQuery is fine, and keep in mind that this should only be shown to non-mobile users, so having some extra dependencies isn't the worst thing.
Francisc
Aug 6, 2017
Good to hear caching and compression are enabled.
However, it's not just download size that matters.
While the file may be cached (never a guarantee), it's parsed and executed by the JS engine every time and takes up memory afterwards.
I realize bitcoin.org isn't a web application where performance is crucial, but why load anything that isn't required? Generally, not just jQuery. Even if you have super-fast internet, last-gen processors and huge SSD drive, I'd still try to deliver the smallest payload possible.
Also, keep in mind this is version 1.11.2 which dates back to December 2014. "Old" is an understatement.
TL;DR:
- I'd use a QR generator that's has no dependencies. If not complicated, I'd do this at build time rather than each time a users opens bitcoin.org.
- I'd use CSS animations and tricks like forcing repaints or
requestAnimationFrame/setInterval. It's just a div sliding down after all. - For XHR, I'd use the lightest possible wrapper for
XMLHttpRequestobject I could find, unless bitcoin.org already has one (which isn't jQuery). - DOM "interrogation" hasn't required jQuery since IE8, just use native methods.
However, if you guys still want to keep jQuery, at the very least update it to latest.
kuzzmi
Aug 6, 2017
•
Contributor
Also, keep in mind this is version 1.11.2 which dates back to December 2014. "Old" is an understatement.
The "old" jQuery is sitting in the repository for two years. It makes absolute sense to upgrade it, however it's out of scope of this PR. And feel free to open a PR with a newer version, this will be highly appreciated.
I'd still try to deliver the smallest payload possible.
That's great! Good for you and I'm all in. Furthermore I totally agree with each point that you raised (except requestAnimationFrame and setInterval for the animations, they are just simple CSS transitions).
TL;DR:
- No doubts there are a lot of things that can be optimized and you always can open a separate PR aside/later to address those issues and do what.
- The work done here is basically just working with what is already present in the repository.
Agree, QR generator could be used without jQuery and DOM modifications could be done with native document and node methods.
If anybody else has the same concerns about performance and minimal footprint of the website, it won't be hard to get rid of jQuery.
| @@ -16,6 +16,8 @@ | ||
| {% if page.lang == 'bg' or page.lang == 'el' or page.lang == 'ko' or page.lang == 'hi' or page.lang == 'pl' or page.lang == 'sl' or page.lang == 'ro' or page.lang == 'ru' or page.lang == 'tr' or page.lang == 'uk' or page.lang == 'zh_CN' or page.lang == 'zh_TW' %}<link rel="stylesheet" href="/css/sans.css">{% endif %} | ||
| <script type="text/javascript" src="/js/base.js"></script> | ||
| {% if page.id != 'download' %}<script type="text/javascript" src="/js/main.js"></script>{% endif %} | ||
| +<script src="/js/jquery/jquery-1.11.2.min.js"></script> | ||
| +<script src="/js/jquery.qrcode.min.js"></script> |
Francisc
Aug 5, 2017
Why not generate QR code at build time rather than every time the modal is opened?
Would spare an extra dependency too.
kuzzmi
Aug 6, 2017
•
Contributor
QR code is regenerated anytime a user selects a sum, and/or enters a message. This can be removed, however there will be no need for quick donation buttons, and for neither custom amount input, nor optional message input. In the end the donation window will contain only static QR code and address.
If this is what we want to show people - a boring modal window, then jQuery and QR code generation and appropriate code can be removed.
Cobra-Bitcoin
Aug 6, 2017
Contributor
I think the interactive way with the QR code being regenerated each time is much better and more likely to produce better results. No need to change anything.
| + <p> | ||
| + {% translate donation-banner-text layout %} | ||
| + </p> | ||
| + <button class="donation-btn" onclick="openDonationModal()"> |
Francisc
Aug 5, 2017
•
Please add type="button" so that it does not default to a submit button.
+ other occurrences in code below.
kuzzmi
Aug 6, 2017
Contributor
type="button" is needed in the forms, otherwise it will default to a submit type and submit a form. In the modal <form> is not needed, thus not used, therefore type="button" not needed either.
Francisc
Aug 6, 2017
Actually, type submit is required in forms since they are the only element for which a submit action makes sense.
Outside of forms, buttons should be just buttons.
Internet Explorer (10- or 9-) will trigger the first submit button found in DOM when pressing enter in an input that is outside of a form.
| + </p> | ||
| + <button class="donation-btn" onclick="openDonationModal()"> | ||
| + {% translate donation-banner-donate-button layout %} | ||
| + </button> |
kuzzmi
Aug 6, 2017
Contributor
Yes, it's used in a handful of pages and in this scenario is used as an entry point for loading BTC/USD rates and setting up event handlers.
| + </div> | ||
| + <div class="donation-visibility-toggle" onclick="toggleDonationBanner()"> | ||
| + {% translate donation-banner-toggle-button layout %} | ||
| + </div> |
Francisc
Aug 5, 2017
•
Please use a button element. It enables keyboard navigation, screen readers handle it properly and is generally semantically correct.
| + | ||
| +<div | ||
| + id="donation-modal" | ||
| + style="display: none" |
Francisc
Aug 5, 2017
Are style attributes common for bitcoin.org?
Otherwise, classes are the way to go.
| + <div class="modal-body bg-white" style="overflow-y:hidden;text-align: center;"> | ||
| + <div> | ||
| + {% for amount in site.donation_banner.amounts_in_usd %} | ||
| + <button class="donation-amount-btn" data-amount-usd="{{ amount }}"> |
| @@ -0,0 +1,160 @@ | ||
| +.donation-btn { | ||
| + background-color: #4CAF50; | ||
| + background-image: -webkit-linear-gradient(bottom, #318f20 14%, #4cad2c 70%); |
Francisc
Aug 5, 2017
•
Missing other vendor prefixes and non-prefixed version.
+ other occurrences in code below.
| + border-radius: 10px; | ||
| + padding: 10px 20px; | ||
| + color: white; | ||
| + font-weight: 600; |
Francisc
Aug 5, 2017
•
Just use bold (or 700). 600 probably doesn't exist and browsers will approximate to bold, or worse, try to improvise the weight.
| +} | ||
| + | ||
| +.donation-modal { | ||
| +/* display: block; */ |
| + | ||
| + amountBtc = parseFloat(amountBtc); | ||
| + | ||
| + if (!isNaN(amountBtc)) { |
Francisc
Aug 5, 2017
•
Check that amountBtc is a number first.
For example, isNaN('') returns false because it first tries to convert to number, than checks NaN.
Alternatively, depending on baseline browser support, use Number.isNaN().
Some comment for a few other places below.
Also, should there be a >0 check?
kuzzmi
Aug 6, 2017
Contributor
Check that amountBtc is a number first.
For example, isNaN('') returns false because it first tries to convert to number, than checks NaN.
A check for being a number is already done using parseFloat:
amountBtc = parseFloat('') // NaN
isNaN(amountBtc) // true
> 0 check is not really necessary as this transaction doesn't make sense. As I will make a few changes, I'll add it.
Francisc
Aug 6, 2017
I missed the parseFloat() statement, sorry about that.
The greater than 0 checks is useful in protecting users from themselves. :)
| + generateDonationQrCode(); | ||
| + }); | ||
| + | ||
| + $('#donation-input-amount-btc').on('keyup', function() { |
| @@ -582,6 +582,14 @@ collections: | ||
| platforms: | ||
| output: false | ||
| +donation_banner: | ||
| + address: 1GwV7fPX97hmavc6iNrUZUogmjpLPrPFoE | ||
| + display: true |
Francisc
Aug 5, 2017
This should also be used for conditionally loading other files (e.g. in index.html).
kuzzmi
added some commits
Aug 6, 2017
|
Looks really good so far, should be ready to be merged soon. Test should pass once you force JSHint to ignore the jQuery files, similar to what was done here. |
|
Thank you. Moving a jQuery QR generator to |
|
Here's a live preview for anyone who wants to check it out: |
Francisc
commented
Aug 6, 2017
|
Checked out the live preview and it looks nice. |
| + &.expanded { | ||
| + margin-top: 0; | ||
| + border-bottom: 4px solid #ef9e5b; | ||
| + box-shadow: 0 5px 10px rgba(0, 0, 0, 0.22); |
Francisc
Aug 6, 2017
I'd offset the shadow 2px lower, in the live preview you can (barely) see the shadow of the pulldown over the expanded banner.
|
@kuzzmi Looks great. Some suggestions:
|
|
Let's avoid pop ups. I think it's OK to have it collapsed in the current state. People just won't click to open/reopen it if they've already donated (or aren't interested). |
kuzzmi
added some commits
Aug 6, 2017
Totally agree, addressed this in a separate commit. I also included Ukrainian and Russian translations for the bar and modal. |
|
Unless others object, this will be merged Monday 7th August. |
Cobra-Bitcoin
added
the
Merge Scheduled
label
Aug 6, 2017
wbnns
merged commit 6f770b3
into
bitcoin-dot-org:master
Aug 7, 2017
1 check passed
kuzzmi
deleted the
kuzzmi:feature-donation branch
Aug 7, 2017
|
@kuzzmi Thanks for all of the work on this. Here's the transaction confirmation for the bounty. |
|
@wbnns Thank you! |
kuzzmi commentedAug 5, 2017
•
edited
This PR improves donations UI and closes #1728.
What was done in this PR:
_config.ymlScreenshots
Any feedback is highly appreciated.
! Warning !
This PR modifies JavaScript and CSS files, which are cached with Nginx with no control over the changes (we've seen that after wallets pages were refactored). This might result in bugs if users get old
main.jsandmain.css.Bounty can be sent to:
14kafbQ3WBmvMhAA3y6gwohUuYQ4zLZrLq