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

refactor: new profile creation page #919

Merged
merged 14 commits into from Jan 7, 2019

Conversation

@luciorubeens
Copy link
Member

commented Jan 2, 2019

Proposed changes

By Oleg.

captura de tela 2019-01-02 as 07 53 05

TODO:

  • Make the instructions side as blocks.
  • Change network selection.
  • Change theme selection.
  • Change the style of the selected background.
  • Optional avatar.
  • Progress bar.

Types of changes

  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)

luciorubeens added some commits Jan 2, 2019

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

Oleg, why this gap between the black and white screen? Doesn't it look better if there is no gap and rounding between them?

@olejegcord

This comment has been minimized.

Copy link

commented Jan 2, 2019

The entire wallet is stacked with blocks that are separated from each other. At the moment, the information unit has a dark color, exactly the same as on the dark version of the wallet. If the blocks are merged, then on the dark version all the blocks will merge into one dark spot and the user's eyes will not be able to separate the information from the block with filling in the information.
In addition, the indentation between the blocks is two times less than the side navigation bar

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

Sorry, I'm not sure I understand you. This is what I meant:

profile

Gap between the left menu bar and the content makes sense. But imo separating the content with a gap looks like they are 2 different pages, even though they're both about creating a profile.

@JeremiGendron

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

I like it a lot

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

However, black and white looks sick and imo combining them looks like the black is the cover of a book (the info) and white is the content. Very good design Oleg!

@olejegcord

This comment has been minimized.

Copy link

commented Jan 2, 2019

and now imagine you've switched your wallet to a dark theme and there's no more separation between black and white and there's only dark and the same color. Does the information block make it even darker? Then we need to make more images for a darker version. I understand what you want and thought about it too, but I made another decision and divided the blocks

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

Well I'm not sure about the correct colors in dark mode, but I'm certain you can make it look good!

However, I'm using dark theme and these two are pretty distinguishable to me:
screenshot 2019-01-02 at 15 08 42

It's up to you, I'm just giving my opinion.

@olejegcord

This comment has been minimized.

Copy link

commented Jan 2, 2019

Thank you for telling us your idea, we will take it into account in future updates and maybe even use it :)

@luciorubeens luciorubeens changed the title refactor: new profile creation page [WIP] refactor: new profile creation page Jan 2, 2019

@luciorubeens luciorubeens changed the title [WIP] refactor: new profile creation page refactor: new profile creation page Jan 3, 2019

src/renderer/i18n/locales/en-US.js Show resolved Hide resolved
src/renderer/i18n/locales/en-US.js Outdated Show resolved Hide resolved
src/renderer/pages/Profile/ProfileNew.vue Outdated Show resolved Hide resolved
@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2019

Also the right part takes up a lot of width when the screen becomes smaller:

From:
schermafbeelding 2019-01-03 om 18 46 38

To this on a slightly smaller screen:
schermafbeelding 2019-01-03 om 18 46 44

And if you reduce the screen width, the application language no longer fits on a single line:

schermafbeelding 2019-01-03 om 18 48 25

@olejegcord

This comment has been minimized.

Copy link

commented Jan 3, 2019

You haven't seen the entire adaptive version. On the definite sizes, the information block is removed.

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2019

@olejegcord this is on an even smaller screen:

schermafbeelding 2019-01-03 om 19 07 08

@olejegcord

This comment has been minimized.

Copy link

commented Jan 3, 2019

This view should be displayed only on the mobile device, preferably on the size of the phone

@luciorubeens

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

@ItsANameToo fixed, take a look

@codecov-io

This comment has been minimized.

Copy link

commented Jan 7, 2019

Codecov Report

Merging #919 into develop will decrease coverage by 0.08%.
The diff coverage is 22.22%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #919      +/-   ##
===========================================
- Coverage    37.83%   37.74%   -0.09%     
===========================================
  Files          193      194       +1     
  Lines         4610     4620      +10     
  Branches       884      885       +1     
===========================================
  Hits          1744     1744              
- Misses        2751     2761      +10     
  Partials       115      115
Impacted Files Coverage Δ
...erer/components/Input/InputGrid/InputGridModal.vue 50% <ø> (ø) ⬆️
.../renderer/components/Selection/SelectionAvatar.vue 40% <ø> (ø) ⬆️
...enderer/components/App/AppSidemenu/AppSidemenu.vue 0% <ø> (ø) ⬆️
...renderer/components/Menu/MenuStep/MenuStepItem.vue 100% <ø> (ø) ⬆️
src/renderer/App.vue 0% <0%> (ø) ⬆️
src/renderer/pages/Profile/ProfileEdition.vue 9.09% <0%> (ø) ⬆️
...renderer/components/Selection/SelectionNetwork.vue 15.38% <15.38%> (ø)
src/renderer/pages/Profile/ProfileNew.vue 7.57% <18.75%> (-2.43%) ⬇️
...c/renderer/components/Selection/SelectionTheme.vue 50% <60%> (-10%) ⬇️
... and 2 more

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 9f4d284...4544986. Read the comment docs.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Looks sexy! It isn't showing extra networks for me though 🤔

image

image

@luciorubeens

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

@alexbarnsley That seems a bug with Object.values on vuex bindings. I solved by removing the computed cache.

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Ah interesting. I'll take another look 👌

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Looks good! I think we can address the responsiveness from the other comments in another PR

@alexbarnsley alexbarnsley merged commit bfabf2e into develop Jan 7, 2019

1 check passed

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

@alexbarnsley alexbarnsley deleted the feat/new-create-profile branch Jan 7, 2019

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

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