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

change avatar preview background to background theme color #4666

Merged
merged 4 commits into from
Sep 27, 2021

Conversation

matthewbcool
Copy link
Contributor

@matthewbcool matthewbcool commented Sep 23, 2021

Avatar-Preview Themed Before Avatar Preview Themed After
avatar pre before avatar pre after

Adds a function to avatar-preview.js that checks for a theme background color (must be hex value) and updates the preview background to that color. Default preview background will remain the same for now.

Putting the Avatar in a generic 'blueprint room' scene may be a better option than using a color there as we can still have contrast issues since the color of the avatar is variable.

For now this PR is meant to bring us one step closer to 'acceptable dark mode'.

Random theme:
anytheme

Default:
default

@brianpeiris
Copy link
Contributor

brianpeiris commented Sep 23, 2021

I don't mean to block a small change like this, but can we get some design input on it? IMO, I think the background served a specific purpose -- it indicated that the avatar preview area was separate from the panel, and indicated an interaction affordance, so that users had a hint that the avatar preview was live and that you could rotate the avatar.

For dark mode, I would rather us use a slightly darker shade or slightly lighter shade, to differentiate it from the background.

@matthewbcool
Copy link
Contributor Author

I don't mean to block a small change like this, but can we get some design input on it? IMO, I think the background served a specific purpose -- it indicated that the avatar preview area was separate from the panel, and indicated an interaction affordance, so that users had a hint that the avatar preview was live and that you could rotate the avatar.

For dark mode, I would rather us use a slightly darker shade or slightly lighter shade, to differentiate it from the background.

It's a good point and valid concern, how about using background3 instead? Matt and I also came to this conclusion.
image

@brianpeiris
Copy link
Contributor

That sounds better, though I wish we gave our theme colors semantic names instead of 1, 2, 3, but that's a separate issue.
I have some code review comments that I'll add shortly.

Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

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

Left some comments about handling edge cases where values are undefined, but looks good to me otherwise.

src/react-components/avatar-preview.js Outdated Show resolved Hide resolved
src/react-components/avatar-preview.js Outdated Show resolved Hide resolved
src/react-components/avatar-preview.js Outdated Show resolved Hide resolved
src/react-components/avatar-preview.js Outdated Show resolved Hide resolved
src/react-components/avatar-preview.js Outdated Show resolved Hide resolved
src/react-components/avatar-preview.js Outdated Show resolved Hide resolved
@matthewbcool matthewbcool removed the request for review from johnshaughnessy September 24, 2021 00:08
Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

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

LGTM. This code is much cleaner. Nice work!

@matthewbcool matthewbcool merged commit 9d6967d into master Sep 27, 2021
@matthewbcool matthewbcool deleted the feature/theme-avatar-preview branch September 27, 2021 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants