Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design improvements #816

Merged
merged 14 commits into from Jan 8, 2019

Conversation

@zillionn
Copy link
Contributor

commented Dec 20, 2018

Proposed changes

  • Edit profile (small screen)
    beforeprofile-edit-before
    afterprofile-edit-after

  • Wallet details (small screen)
    beforesmall-wallet-before
    aftersmall-wallet-after

  • Wallet details (large screen)
    before1920-wallet-before
    after1920-wallet-after

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@j-a-m-l j-a-m-l self-requested a review Dec 20, 2018

@j-a-m-l
Copy link
Contributor

left a comment

  • The gap between the 2 parts of the network menu on the sidebar is on purpose.
  • The changes to the transaction table don't solve the problem with truncating 2 times (Ajsh...3x...) the recipient on the Dashboard
  • Using 8 character instead of 6 truncates (2 times) the wallet name on the sidebar on some resolutions.

Apart from that, could you change the profile edition to display the avatars on 2 rows when the screen is too small?

@zillionn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

I'm sorry I don't really understand you... If you don't like these changes, please just close the PR. Thanks.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

Which points don't you understand?

@zillionn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

All of them don't make sense to me. Let me explain you my changes more thoroughly:

  • Edit profile - The whole point is to remove the scrollbar and show the Save button because now it is not even visible.
  • Network settings - I don't see any point for this gap, it aways hurts my eyes because of the content underneath. I'm certain it looks better without the gap, ask Oleg.
  • Wallet details - I've made 2 changes:
    1. The hover on the right side menu is supposed to add a red line on the left side of every button but it doesn't for some reason (https://streamable.com/81rg0) with the current width w-32 and it is also too narrow for large screens, so I've changed it to percent w-1/7 (12.5%).
    2. Made the Smartbridge row wider in percents w-1/4 (25%) because the ID row is too wide and it's taking too much space even on small screens as you can see on the screenshots.
@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

  • I saw that you removed the truncating: good.
  • The save button can be reached by scrolling, so, since you have started changes to make it more visible, I asked you to improve the 4 profile avatars too on small screens.
  • About the network settings, Oleg's response is "no, I specially separated the buttons, what would people understand where the buttons and where not"
  • About the changes to the transaction table: they are useful, but they don't solve the problem with how the sender and recipient are truncated, which is important too.
  • About the red line on big screens, I've tried on 4K and I can see it perfectly.
@zillionn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

  • How to improve the avatars? If I place them on 2 lines the scrollbar will be visible again. The whole point is to move everything up and remove the scrolling on small screens.
  • OK, it's up to Oleg but this gap is hurting my eyes.
  • Oh, I understand what you mean now but I don't see any truncated Sender and/or Recipient.
  • Red line on small screens, not large. I'm on Macbook Pro laptop (1280x800) and don't see the line, please check the video - https://streamable.com/81rg0

zillionn added some commits Dec 21, 2018

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

@j-a-m-l are you happy with these changes now?

@codecov-io

This comment has been minimized.

Copy link

commented Jan 8, 2019

Codecov Report

Merging #816 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #816   +/-   ##
========================================
  Coverage    37.74%   37.74%           
========================================
  Files          194      194           
  Lines         4620     4620           
  Branches       887      885    -2     
========================================
  Hits          1744     1744           
  Misses        2761     2761           
  Partials       115      115
Impacted Files Coverage Δ
src/renderer/pages/Wallet/WalletShow.vue 14.28% <ø> (ø) ⬆️
src/renderer/pages/Profile/ProfileEdition.vue 9.09% <ø> (ø) ⬆️
...nderer/components/Transaction/TransactionTable.vue 18.18% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfabf2e...665fc66. Read the comment docs.

@j-a-m-l

j-a-m-l approved these changes Jan 8, 2019

@j-a-m-l j-a-m-l merged commit 366664e into ArkEcosystem:develop Jan 8, 2019

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details

PHANTOM-DEV1 added a commit to PhantomChain/desktop-wallet that referenced this pull request Jan 17, 2019

@zillionn zillionn deleted the zillionn:design-improvements branch Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.