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

feat: optional profile avatar #947

Merged
merged 14 commits into from Jan 15, 2019

Conversation

@luciorubeens
Copy link
Member

commented Jan 9, 2019

Proposed changes

This PR introduce the ability not to choose an avatar for the profile, instead the first letter of the profile name will be displayed.

button-letter

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)

@luciorubeens luciorubeens requested review from j-a-m-l and alexbarnsley Jan 9, 2019

@boldninja

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

@luciorubeens did we add this switch also on Profile creation page?

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Looks very good, not sure what's the point of the switch, but it's not that important I guess.

I kind of don't like the description on the left side:
Enter your name or nickname and select your preferred language and default currency.
How about?
Enter your name/nickname, select preferred language and default currency.

@luciorubeens

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@boldninja Yes, the switch is on both pages, profile creation and editing.

@boldninja

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

My man :yesyes:

@luciorubeens

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@zillionn It's for people who want to select an avatar. And yes, I'll change the instruction text.

captura de tela 2019-01-09 as 11 24 51

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@luciorubeens I meant that you can just remove the switch and show all avatars but the default one to be the first letter. And Avatar for your profile could become Choose a profile image or something.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

I think that @zillionn idea would be the best approach, @luciorubeens. In fact, it could be used to include the network icon as another avatar, as other users have suggested.

luciorubeens added some commits Jan 10, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jan 14, 2019

Codecov Report

Merging #947 into develop will decrease coverage by <.01%.
The diff coverage is 39.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #947      +/-   ##
===========================================
- Coverage    37.59%   37.58%   -0.01%     
===========================================
  Files          194      196       +2     
  Lines         4695     4709      +14     
  Branches       901      905       +4     
===========================================
+ Hits          1765     1770       +5     
- Misses        2811     2820       +9     
  Partials       119      119
Impacted Files Coverage Δ
...erer/components/Input/InputGrid/InputGridModal.vue 50% <ø> (ø) ⬆️
...derer/components/Selection/SelectionBackground.vue 100% <ø> (ø) ⬆️
...enderer/components/App/AppSidemenu/AppSidemenu.vue 0% <ø> (ø) ⬆️
src/renderer/models/profile.js 0% <ø> (ø) ⬆️
src/renderer/components/Input/InputField.vue 100% <ø> (ø) ⬆️
src/renderer/pages/Profile/ProfileNew.vue 7.57% <0%> (ø) ⬆️
...derer/components/Input/InputGrid/InputGridItem.vue 40% <0%> (ø)
.../renderer/components/Input/InputGrid/InputGrid.vue 37.5% <0%> (-3.68%) ⬇️
src/renderer/pages/Profile/ProfileEdition.vue 9.09% <0%> (ø) ⬆️
src/renderer/pages/Wallet/WalletAll.vue 61.9% <100%> (ø) ⬆️
... and 13 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 e7bdea9...651e962. Read the comment docs.

@luciorubeens

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@j-a-m-l @alexbarnsley Changes implemented, can you take a look?

@j-a-m-l
Copy link
Contributor

left a comment

I'd change this:

screenshot_20190114_191617

Maybe moving the "No Avatar" label above the check icon, or to the top

@alexbarnsley

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

I agree with @j-a-m-l's comment, maybe move it up to the bottom of the circle if possible? Like it does on the actual avatars. Failing that, moving the text up.

Oleg, thoughts?

luciorubeens and others added some commits Jan 15, 2019

@j-a-m-l
Copy link
Contributor

left a comment

It's OK. As an optional suggestion, @luciorubeens I'd use a lighter colour for "No avatar" or blue or something like that, to differentiate it more from the headers ("Avatars" and "Select avatar").

@luciorubeens

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@j-a-m-l Agreed. Done

@j-a-m-l j-a-m-l merged commit b785613 into develop Jan 15, 2019

1 check passed

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

@j-a-m-l j-a-m-l deleted the feat/button-letter branch Jan 15, 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.