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

[SG-58] Avatar color selector #3691

Merged
merged 52 commits into from
Jan 1, 2023
Merged

[SG-58] Avatar color selector #3691

merged 52 commits into from
Jan 1, 2023

Conversation

BrandonM-Bitwarden
Copy link
Contributor

@BrandonM-Bitwarden BrandonM-Bitwarden commented Oct 5, 2022

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

We want to add the ability for users to select the avatar color that will represent an account in the header of the app, so that they can better distinguish between accounts with similar or the same initials.

Code changes

Backend: bitwarden/server#2330

+ dynamic-avatar.component – A component that encapsulates the bit-avatar component and listens for changes in the avatar color values via the AvatarService.
+ selectable-avatar.component – A component that encapsulates the bit-avatar component and reacts to click events by toggling their selection state and firing events.
~ navbar.comonent – Changed bit-avatar to dynamic-avatar so it can react to changes.
+ change-avatar.component – The components that gets put into a modal and allows users to pick avatar colors as needed.
~ profile.component - Changed bit-avatar to dynamic-avatar so it can react to changes and added a button to open the change-avatar modal.
~ loose-components.module – Added needed component references.
~ organization-name-badge.component – Made color rely on a state lookup and fallback to default.
~ messages.json – language stuff.
~ jslib-services.module – Added new services references.
+ avatar-update.service – Added service that handles loading and saving colors from state and api along with emitting events.
~ api.service – Added new avatar color API endpoints.
~ state.service – Added new avatar color state endpoints.
~ utils.ts – Added RegEx Validator for hex colors.
~ account.ts – Added color property.
+ update-avatar.request – Added request body model for API endpoint save call.
~ profile.response – Added color property.
~ avatar.component – Added more sizes and title input.

Screenshots

chrome_5YNFJhjWCM
Button to open modal

chrome_2jg2iCiTSY
Avatar change modal

chrome_ot1HiOjtPD
Custom color picker

chrome_o3atJpgh1A
Avatar colors

chrome_ROvPPDpMRz
Colors used elsewhere!

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

Copy link
Member

@differsthecat differsthecat left a comment

Choose a reason for hiding this comment

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

I'm not seeing this change here, but can you update the OrganizationNameBadgeComponent to use the color selected by the user?

@Hinton
Copy link
Member

Hinton commented Oct 24, 2022

I skimmed the PR, since I've migrated the old avatars to the new component library component. Some things to consider:

  1. Should the users custom color be visible to everyone or only themselves?
    • If only for themselves, how do we handled it in user lists like organization people?
    • If not only for themselves, we need a way to send in the required data vs fetching it from state
  2. To me it feels like we should be building a new "smart" component on top of the avatar component. The avatar component currently handles a few different use cases.
    • Their own
    • Other users
    • Organizations
    • Providers
  3. Ideally components in the component library should depend on as little as possible and be purely presentational.

Copy link
Member

@differsthecat differsthecat left a comment

Choose a reason for hiding this comment

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

There's a failing build on CLI because of the addition of avatar service to sync service; I'd try setting the value in state service instead of injecting the avatar service here.

differsthecat
differsthecat previously approved these changes Nov 17, 2022
Copy link
Member

@differsthecat differsthecat left a comment

Choose a reason for hiding this comment

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

Looks good @BrandonM-Bitwarden !

Hinton
Hinton previously requested changes Nov 17, 2022
@Hinton Hinton dismissed their stale review November 21, 2022 10:01

Relevant blockers have been resolved. Thanks.

differsthecat
differsthecat previously approved these changes Nov 21, 2022
@trmartin4 trmartin4 added the needs-qa Marks a PR as requiring QA approval label Nov 28, 2022
* SG-920 + SG-921

* Update change-avatar.component.html

* Update selectable-avatar.component.ts
… the Account Settings menu (#4359)

* SG-926

* fix: comment

* fix: undo

* fix: imp
@trmartin4
Copy link
Member

Approving this to be PRed into master, as it has been fully tested in the changes/SG-58 branch.

@trmartin4 trmartin4 self-requested a review January 1, 2023 15:28
@trmartin4 trmartin4 merged commit d41b3b1 into master Jan 1, 2023
@trmartin4 trmartin4 removed the needs-qa Marks a PR as requiring QA approval label Jan 1, 2023
rr-bw pushed a commit that referenced this pull request Jan 10, 2023
* changes

* merge

* undo

* work

* stuffs

* chore: added custom color picker

* oops

* chore: everything but the broken sink

* picker v2

* fix: cleanup

* fix: linty

* fix: use tailwind

* fix: use tailwind

* undo: merge error

* remove: old color picker

* fix: merge issue

* chore: use input vs component

* fix: move logic out!

* fix: revert changes to bit-avatar

* fix: cleanup undos

* feat: color lookup for "me" badge in vault

* fix: naming stuff

* fix: event emitter

* fix: linty

* fix: protect

* fix: remove v1 states
work: navatar

* fix: big

* fix: messages merge issue

* bug: differing bg colors for generated components

* feat: added sync stuff

* fix: cli

* fix: remove service refs, use state

* fix: moved from EventEmitter to Subjects

* fix: srs

* fix: strict stuff is nice tbh

* SG-920 + SG-921 (#4342)

* SG-920 + SG-921

* Update change-avatar.component.html

* Update selectable-avatar.component.ts

* [SG-926] [SG-58] [Defect] - Selected Avatar color does not persist in the Account Settings menu (#4359)

* SG-926

* fix: comment

* fix: undo

* fix: imp

* work: done with static values (#4272)

* [SG-35] (#4361)

Co-authored-by: Todd Martin <[email protected]>
rr-bw pushed a commit that referenced this pull request Jan 10, 2023
* changes

* merge

* undo

* work

* stuffs

* chore: added custom color picker

* oops

* chore: everything but the broken sink

* picker v2

* fix: cleanup

* fix: linty

* fix: use tailwind

* fix: use tailwind

* undo: merge error

* remove: old color picker

* fix: merge issue

* chore: use input vs component

* fix: move logic out!

* fix: revert changes to bit-avatar

* fix: cleanup undos

* feat: color lookup for "me" badge in vault

* fix: naming stuff

* fix: event emitter

* fix: linty

* fix: protect

* fix: remove v1 states
work: navatar

* fix: big

* fix: messages merge issue

* bug: differing bg colors for generated components

* feat: added sync stuff

* fix: cli

* fix: remove service refs, use state

* fix: moved from EventEmitter to Subjects

* fix: srs

* fix: strict stuff is nice tbh

* SG-920 + SG-921 (#4342)

* SG-920 + SG-921

* Update change-avatar.component.html

* Update selectable-avatar.component.ts

* [SG-926] [SG-58] [Defect] - Selected Avatar color does not persist in the Account Settings menu (#4359)

* SG-926

* fix: comment

* fix: undo

* fix: imp

* work: done with static values (#4272)

* [SG-35] (#4361)

Co-authored-by: Todd Martin <[email protected]>
cyprain-okeke pushed a commit that referenced this pull request Jan 11, 2023
* changes

* merge

* undo

* work

* stuffs

* chore: added custom color picker

* oops

* chore: everything but the broken sink

* picker v2

* fix: cleanup

* fix: linty

* fix: use tailwind

* fix: use tailwind

* undo: merge error

* remove: old color picker

* fix: merge issue

* chore: use input vs component

* fix: move logic out!

* fix: revert changes to bit-avatar

* fix: cleanup undos

* feat: color lookup for "me" badge in vault

* fix: naming stuff

* fix: event emitter

* fix: linty

* fix: protect

* fix: remove v1 states
work: navatar

* fix: big

* fix: messages merge issue

* bug: differing bg colors for generated components

* feat: added sync stuff

* fix: cli

* fix: remove service refs, use state

* fix: moved from EventEmitter to Subjects

* fix: srs

* fix: strict stuff is nice tbh

* SG-920 + SG-921 (#4342)

* SG-920 + SG-921

* Update change-avatar.component.html

* Update selectable-avatar.component.ts

* [SG-926] [SG-58] [Defect] - Selected Avatar color does not persist in the Account Settings menu (#4359)

* SG-926

* fix: comment

* fix: undo

* fix: imp

* work: done with static values (#4272)

* [SG-35] (#4361)

Co-authored-by: Todd Martin <[email protected]>
@BrandonM-Bitwarden BrandonM-Bitwarden deleted the change/SG-58 branch March 16, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants