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

Wallets: show up to 14 wallets #1592

Merged
merged 4 commits into from May 14, 2017

Conversation

Projects
None yet
4 participants
Contributor

harding commented May 9, 2017

Closes #1569

Preview: http://dg0.dtrt.org/en/choose-your-wallet

This uses @Mirobit's suggestion from the above issue to expand the display to show up to 14 wallets at a time, which is the maximum number of entries we have in any category except for the main page (which contais 18 wallets).

2017-05-09-15_08_33_610912063

I've tested on Firefox 45 ESR for Linux, Chromium for Linux, and Chrome for Android. Testing on other browsers appreciated.

In addition, wallet additions made after this PR is merged will cause a test to fail if any category begins to contain more than 14 wallets. That way we shouldn't be surprised by wallets not being shown in the future. We can discuss what to do about the problem then.

As part of the above, I made minor fixes to the entries for Electrum and Coin.Space that caused not all of their information to be displayed all of the time. I added setting up JSON schema checker to my low-priority todo list so we can catch more of these sort of problems with automated testing in the future.

harding added some commits May 9, 2017

Wallets: produce error if mismatch between platform and compats
If a wallet contains a section for a particular platform, it must also
contain that platform in its compatibility field.

This commit adds a test for the above and corrects two existing
mismatches.
Contributor

Mirobit commented May 9, 2017

@harding Thanks for picking that up. LGTM.

Contributor

crwatkins commented May 9, 2017

Thanks @harding for this badly needed update. Also thanks for the added sanity checks; I've been concerned about those interactions for a while now.

I've tested on Chrome and Safari browsers on iPhone, iPad, and Mac OS. I noticed a missing wallet in certain UX sequences on iOS (and Android) that I need to research more and submit a separate issue since these same problems are reproducible on the currently deployed bitcoin.org and do not seem to be related to this update.

LGTM.

Contributor

harding commented May 10, 2017

Looked into the issue @crwatkins mentioned (thanks!) and opened issue #1593 for it since it's not a bug introduced by this branch. I'll try to look into more later today.

Contributor

wbnns commented May 13, 2017

Thanks all. Unless others object, this will be merged on Sunday, May 14th (tomorrow).

@wbnns wbnns self-assigned this May 13, 2017

@wbnns wbnns merged commit 9bcf99e into bitcoin-dot-org:master May 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment