Skip to content
This repository was archived by the owner on Dec 3, 2021. It is now read-only.

Comments

New design part I.#30

Open
ajandera wants to merge 6 commits intostatus-im:developfrom
ajandera:develop
Open

New design part I.#30
ajandera wants to merge 6 commits intostatus-im:developfrom
ajandera:develop

Conversation

@ajandera
Copy link

Changes of css, main page and wallet detail without transactions.

@GustavoNunes GustavoNunes self-requested a review June 2, 2017 19:52
@GustavoNunes
Copy link

wallet-review

  1. The icon is too big here. It is not aligned with the content area. It is over the title.
  2. The arrow in the icon should be white.
  3. We should deal with lack of space here in tiny screens. I think we can remove 2 of these zeroes. Then we can remove the currencies (ETH, USD, etc). If the wallet name is still too big, we should crop it and add an ellipsis (...).
  4. The labels aren't centered, the buttons are too big.
  5. Lack of alignment / centralization.
  6. Same issues as 3, but the font-size here is bigger than it should be.

Copy link

@GustavoNunes GustavoNunes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments regarding conventions and maintainability. Also, if you are having trouble with the positioning, consider using flexbox instead of floats and clearfixes.



.slick-slider{
margin-bottom: 0 !important;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets avoid !important here and elsewhere unless absolutely necessary.

font-size: 20px;
}
.wallet-amount p.left-block {
flot:left

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "flot"

send-amount (subscribe [:get :send-amount])
request-amount (subscribe [:get :request-amount])
text (subscribe [:get :text])]
text (subscribe [:get :text])]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the previous indentation, it reads better.


(defn wallet-uri [wallet-id]
[:a.wallet-btn {:href (str "#/wallet/" wallet-id)} "Open wallet"])
(defn allTransactions []

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use all-transactions here. Refer to https://github.com/bbatsov/clojure-style-guide#lisp-case.

[:span.wallet-currencies
[:div.currency-usd
(to-fixed (:amount balance-fmt) 4) [:span.cur (:unit balance-fmt)]]]
] [:div.clearfix]]))))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These clearfix forms, here and elsewhere, should be indented like the others.

[:div.wallet.manage-wallet
[:div.wallet-name "Manage wallets"]
[:span.wallet-points
[:div.point] [:div.point] [:div.point]]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation of the ellipsis is not responsive. Try using a single element as an image, svg or even text ("...").

@GustavoNunes
Copy link

Thanks for the changes @ajandera! Here are some more issues I found while testing in a small screen.

wallet-review-2

  1. There is too much letter spacing in the WALLET title.
  2. The lines between wallets and around the buttons are too thick, and with the wrong color / opacity.
  3. There is a big empty gap above "Manage wallets".
  4. Note how the ellipsis icon is weird-looking (spaced apart). Instead of using three separate divs, one for each point, please use an image (png or svg). Here is the link to the icon iconDotsHorizontalWhite_2017-06-09.zip
  5. The vertical padding in the wallet list item needs to be slightly bigger.
  6. There is too much spacing before the currency.
  7. Same as above.
  8. The wallet name should be in bold font.
  9. The address isn't fitting. Something like overflow: hidden and text-overflow: ellipsis should take care, but please do test it.

@ajandera
Copy link
Author

Hi Gustavo, i updated a PR, please look at it, can you please send me the images for transaction icons? I update transaction history part and need the icon to finish, or is there a way to get icons from zeplin?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants