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

Refactor wallet listings and categories into individual web pages #1691

Merged
merged 37 commits into from Jul 31, 2017

Conversation

Projects
None yet
4 participants
Contributor

kuzzmi commented Jul 18, 2017 edited

First, sorry for such a big PR, I know it will not be that easy to review.

Here are some screenshots:

http://bitcoin.org/de/wallets/desktop/windows/bitcoincore/ will look like this:

image

http://bitcoin.org/de/wallets/desktop/linux/ will look like this:

image


I'll try to explain what was done to resolve #1661.

  • refactor wallets data to separate Jekyll collection, so it's easier to maintain wallets
  • reduce duplication of data of wallets' data
  • enable individual pages for each 3-tuple of (platform, os, wallet) and tuple of (platform, os)
  • add plugin to make permutations of translations and new pages
  • create _layouts/wallet-container.html _layouts/wallet-platform-container.html _layouts/wallet-platform.html to support wallet and platform models and enforce re-usability of layout
  • create _includes/layout/base/wallets-list.html and _includes/layout/base/wallets-menu.html to enforce re-usability
  • preserve "rotation" of wallets on the platform pages
  • add links to wallet page to navigate to specific OS
  • refactor _templates/choose-your-wallet.html to use updated layouts
  • refactor wallet-related styles from _sass/screen.scss to a separate file
  • refactor wallet-related styles to use SASS features to enforce more transparent styling and styles generation
  • align wallet names in the translation to reduce confusion (walletbitcoinqt -> walletbitcoincore)
  • add icons for hardware and web to display on the wallet page
  • make some small code style fixes like removing of trailing spaces etc
  • rework YAML validation scheme
  • run make travis locally without errors'
  • add new Makefile script to test empty title tags

All feedback and comments are welcome.

If this works for you, here is my address:
1GomWkkJwg9WSqiGWjvMGUP3UFN3KVxfCw

kuzzmi added some commits Jul 15, 2017

Contributor

kuzzmi commented Jul 18, 2017

Ok, I see the problem with YAML schema. Will push the changes once complete.

Contributor

crwatkins commented Jul 18, 2017

@kuzzmi Looks like a lot of good work here!

Just a couple quick comments on first glance:

  • The pages generated for wallets have empty html titles, e.g. <title></title>. We probably want page titles.

  • Have you tried on iOS? The touch interface is a little strange. Sometimes when clicking on one menu (e.g. Mobile) another menu will come down (e.g. Desktop). Also, there is some awkward scrolling when making selections, and multiple scoring popups can appear up at the same time, sometimes popping under a previous popup.

Contributor

kuzzmi commented Jul 18, 2017 edited

@crwatkins Thank you!

Unfortunately, I don't have any working iOS device :(
I'll update the titles and will take a look at the mobile version, but for iOS part, it will take a few days for me to find a device for debugging.

Contributor

crwatkins commented Jul 19, 2017

I've noticed the exact same behaviors on Android as on iOS (both using Chrome).

@wbnns wbnns self-assigned this Jul 19, 2017

Contributor

kuzzmi commented Jul 19, 2017

Ok, I've found the issues you were talking about and will soon update this PR

Contributor

kuzzmi commented Jul 19, 2017 edited

The last two commits should fix all the issues on mobile devices brought by this refactor.

Contributor

crwatkins commented Jul 19, 2017

Thanks. I think the issues I mentioned above are fixed. I think there are still some issues with the transitions of the wallet listings. For example, if on mobile you select Mac, then select Hardware, the wallet listing does not switch to hardware wallets.

Contributor

wbnns commented Jul 20, 2017 edited

@kuzzmi Thanks for all of the hard work on this! I'm currently working through this review. At the moment I'm seeing some layout issues (13" Macbook Pro, Chrome 59).

Example: https://bitcoin.cryptopelago.com/en/wallets/desktop/windows/

image

Also on the individual pages: https://bitcoin.cryptopelago.com/en/wallets/desktop/windows/bitcoincore/

image

I was able to reproduce these layout issues on my laptop Firefox 54 and Safari 10 as well.

If anyone is interested in a live preview, one is available here: https://bitcoin.cryptopelago.com/

Contributor

kuzzmi commented Jul 20, 2017 edited

Thank you, @wbnns!
I will spin-up a VM with MacOS to take a look. It doesn't feel any close to what should be there.

I'll try to push the changes back as soon as I can find the cause of that. Have you tried to clear your cache?

Contributor

kuzzmi commented Jul 20, 2017

This starts looking interesting. Tested all three:

image

image

@wbnns wbnns added the Under Review label Jul 21, 2017

Contributor

wbnns commented Jul 23, 2017

@kuzzmi Hey, sorry for the false alarm - after clearing my cache the issue was resolved. 👍

Contributor

schildbach commented Jul 24, 2017

Note that with this PR, the category order changed from "mobile desktop hardware web" to "desktop hardware mobile web". I think such a change should be in a separate PR and discussed out of the context of this PR.

I also think the row of categories should stay at the top, even if a wallets details are shown.

Icons now do not appear in a grid if there is an uneven number of wallets in the displayed category. For these cases, it looks a bit less nice imho.

Contributor

wbnns commented Jul 24, 2017

@kuzzmi Great job bringing all of this together. This should improve SEO on the wallets significantly and increase traffic to the site by making them accessible to search engines. People will also now be able to link others/share individual wallet links on Bitcoin.org across the web. 👍

Unless others object, this will be merged on Thursday, July 27th.

@wbnns wbnns added Merge Scheduled and removed Under Review labels Jul 24, 2017

Contributor

kuzzmi commented Jul 27, 2017

@wbnns Thank you! I just realized, there are still some details left, like instructions about adding a new wallet.
Over the weekend I'll update the document to align it with a new way of submitting a wallet.

@wbnns wbnns added On Hold and removed Merge Scheduled labels Jul 28, 2017

Contributor

wbnns commented Jul 28, 2017

@kuzzmi No prob! Good catch, thanks for thinking about the documentation to help others. I'll put this on hold until you're done.

Contributor

kuzzmi commented Jul 28, 2017

@wbnns apparently just a few changes were needed, should be fine now.

Contributor

crwatkins commented Jul 28, 2017

@kuzzmi In the instructions, I believe

Entries are ordered by levels and new wallets must be added after the last wallet on the ordered by levels and new wallets must be added after the last wallet on the same level.

is no longer relevant. Correct?

Updates wallets' management documentation
This commit removes ordering instruction, as wallets are stored in individual files.
Adds Level property description section to the "Adding a wallet" section.
Contributor

kuzzmi commented Jul 28, 2017

@crwatkins good catch, that's correct. I removed this part and added a bit clearer instructions on adding a wallet, including better explanation of what is level property in the wallet file.

Contributor

crwatkins commented Jul 28, 2017

Thanks. That's much better. It's interesting to see that GitHub can now display most of the wallet files as Markdown. It's a shame that it won't render the wallets with the DEFAULT entries. I'm not requesting a change, but if you had a simple way to fix that, it would be cool.

Contributor

kuzzmi commented Jul 28, 2017 edited

@crwatkins Oh, I haven't seen this. This is a positive side effect of this change. The problem is that the DEFAULT is used as a link/anchor feature from YAML to reuse some parts of the file.

The reason why GitHub can render a wallet file as a table, is that GitHub is an official supporter of Jekyll, which uses own "extended" version of Markdown with a support of YAML. The fact is that this is not a valid Mardown file, it just happened that GitHub can render files created for Jekyll.

There won't be an easy fix to this, as it will require to update the wallet structure, the schema, and the layout files.

@wbnns wbnns added Merge Scheduled and removed On Hold labels Jul 28, 2017

Contributor

wbnns commented Jul 28, 2017

Unless others object, this will be merged on Sunday, July 30th.

@wbnns wbnns merged commit 103b1b3 into bitcoin-dot-org:master Jul 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Contributor

wbnns commented Jul 31, 2017

@kuzzmi Thanks again for all of the work on this! Here's the transaction confirmation for the bounty.

@kuzzmi kuzzmi deleted the kuzzmi:wallets-pages branch Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment