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

Added KnC Wallet for Android #493

Merged
merged 1 commit into from Aug 12, 2014

Conversation

Projects
None yet
3 participants
Contributor

LinusU commented Aug 1, 2014

KnC Wallet builds upon the strong foundation provided by Andreas Schildbach's Bitcoin Wallet. On top of that we added an easy to use directory feature, which allows you to link your phone number and your wallet address together. This makes it possible to send bitcoins directly to the people in your contacts list, without having to worry about addresses or qr codes.

We believe in making bitcoin accessible for non-technical people and this wallet is a great first wallet for people wanting to try on bitcoin.

In the background, the app uses the well tested bitcoinj library and talks directly with the bitcoin network.

Please ask if you have any questions regarding the app, I should be able to answer anything about it, including highly technical questions.

Preview of the additions:
KnC Wallet

Contributor

schildbach commented Aug 1, 2014

I think quadratic icons should be a bit smaller than circular ones. I'm not sure about the right proportion, probably a quadrat should have about the same number of pixels as a circle (Squaring the circle, you know?). Irregular shapes would fall somewhere inbetween.

If you look at the current Coinkite icon that's too massive as well.

The Android style guide has a rule for this -- maybe that would be a good start (I used it when I redid all of the icons 2 years ago).

Contributor

LinusU commented Aug 1, 2014

Sounds good, just pushed a version where the size of the icon is reduced from 96px to 85px.
(sqrt(pi)*(96/2))

Preview:
KnC Wallet, smaller icon

Contributor

schildbach commented Aug 1, 2014

Thanks! That looks good to me.

Contributor

saivann commented Aug 1, 2014

@schildbach @LinusU Although I like the idea, maybe we should agree with some clear rules and apply this rule to all wallets in a separate pull req? Do you have some idea about what we could use to detect the number of pixels and calculate the right height/width ratio? I am not sure if we have access to high-res icons for all wallet so we can resize them all correctly.

If only KnC is using a reduced size icon, this isn't very fair and consistent. Hive, Armory, Blockchain and Coinkite would likely need to be resized too. If that's complicate, I think just sticking to 96 X 96px isn't so bad either (more consistent alignment, less consistent sizes).

Contributor

saivann commented Aug 1, 2014

@LinusU kncwallet.com doesn't support https, maybe we could link to the app on Google Play instead? https://play.google.com/store/apps/details?id=com.kncwallet.wallet

Otherwise the pull request LGTM. I'll let some time for people to comment before suggesting this pull request to be merged. Thanks!

Contributor

schildbach commented Aug 3, 2014

Hive is quite circular, I don't think it needs any change. Coinkite I mentioned already is too fat. I will submit a PR.

Contributor

saivann commented Aug 3, 2014

@schildbach Thanks!

Contributor

LinusU commented Aug 4, 2014

Took the liberty to experiment a bit with the different icons. I think that the only ones that needs to change is Armory and Coinkite. Also, Coinbase is just a few pixels to small and Mycelium could possible move a few pixels up.

icons

@schildbach Your icon isn't retina enabled (I assume that is why we have 144x144), would be nice if you have a larger version.

I propose that we keep the KnC Wallet icon at 85x85px and then I can try and get some pull requests for Armory and Coinkite. Then the guidelines is to keep circular icons at 96x96 and square icons at 85x85. Sounds good?

Regarding the link to kncwallet.com, dose it matter that it isn't https? It's only a promotional website and no login or sensitive information is going over the wire. If you only want to link to https I will try to get a certificate up and running for the site asap.

Contributor

schildbach commented Aug 4, 2014

@LinusU Are you aware of ongoing progress at bitcoin#498 (comment) ? We resized Armory and Coinkite already. You're missing Xapo which has been added recently.

I'll have a look at the Bitcoin Wallet icon.

Contributor

LinusU commented Aug 4, 2014

Oh, sorry, I completely missed that. Nice! Then I definitely leave the KnC one at 85x85.

The Armory icon is still a bit outside the outer 96x96 box thought.

screen shot 2014-08-04 at 13 03 24

Contributor

LinusU commented Aug 4, 2014

Also fixed the multibit icon in #504.

Contributor

saivann commented Aug 4, 2014

@LinusU Thanks for your help with icons! The Armory icon that I've merged yesterday should fit within 96px. Regarding the new source: field, yes, it would be nice if you add it to your pull request.

Contributor

schildbach commented Aug 4, 2014

@LinusU The Armory icon is quite irregular. I think because it doesn't use most of the corners (of the bounding quadrat) it can be allowed to use a little bit outside in the middles. Same for blockchain.info: It's a bit narrower than the box but a bit taller as well. In the end, you need to try and see how it looks with these cases.

Contributor

LinusU commented Aug 5, 2014

Absolutely, I was just thinking that the armory icon should be inside the 96x96 box.

I rebased on master and added in the source: field.

Contributor

LinusU commented Aug 8, 2014

@saivann Are we ready to merge this or is there anything more you want me to do?

Contributor

saivann commented Aug 8, 2014

@LinusU I've just tested your wallet and noticed it requires the user to reveal their phone number (and addresses are disclosed to a central server according to the FAQ), so you would need to replace checkfailprivacydisclosurespv by checkfailprivacydisclosureaccount in your privacy score.

Otherwise this LGTM.

Feel free to do it or let me take care of it, but your icon needs to be moved under wallets/kncwallet.png and the picture needs to be cut to 96px instead of 144px, just removing useless extra transparent space in the picture (the icon itself should remain 85px).

Contributor

saivann commented Aug 10, 2014

In the absence of critical feedback, this pull request will be merged on August 12th.

Contributor

LinusU commented Aug 11, 2014

  • s/http/https - We fixed a certificate :)
  • s/checkfailprivacydisclosurespv/checkfailprivacydisclosureaccount/
  • Move icon to wallets/
  • Crop icon to 96x96
  • Rebased on master

All the other icons are still 114x114 in master, cropping it to 96x96 made it look really bad. What am I missing?

Contributor

saivann commented Aug 11, 2014

@LinusU Yes sorry, this change has been reverted. Your current icon is fine, thanks!

@saivann saivann merged commit 6d5cbbf into bitcoin-dot-org:master Aug 12, 2014

Contributor

LinusU commented Aug 12, 2014

Thank you!

@LinusU LinusU deleted the LinusU:knc branch Aug 12, 2014

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