-
Notifications
You must be signed in to change notification settings - Fork 841
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
Added more customizations to Avatar #903
Conversation
|
||
@if $type == 'light' { | ||
$color: rgba(255, 255, 255, $alpha); | ||
$color: rgba($euiColorEmptyShade, $alpha); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snide I'm not sure if these are breaking changes or not since it's changing out a hard-coded color for a variable, though this variable changes depending on the theme. The mixin is currently only used in the avatar SASS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snide would you consider this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Think you're good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Only question is whether you think we should be this loose with the initials. For example, "Ki" vs. "KI" vs. "ki". Should the capitalization be defined by the type itself? RIght now there's room for variance depending upon how they format the string.
Just posing the question. Don't know that I have a good answer. This is certainly the most flexible so it's fine. It's also something we can change later if abused.
src/components/avatar/avatar.js
Outdated
{...rest} | ||
> | ||
{optionalInitial} | ||
</div> | ||
); | ||
}; | ||
|
||
function checkValidColor(props, propName, componentName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a todo comment in here (or just make an issue) to migrate to the check color service once the color picker goes in. I spent some time making this stuff generic, and it'll be easy to switch out.
font-size: $euiSizeL; | ||
} | ||
// Modifiers for sizing. | ||
$avatarSizing: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better
I thing the flexibility is fine here and don't feel like it will get abused when there's only 2 characters to available. |
@chandlerprall I'm going to merge this in since I know you're busy this week, maybe you can take a quick post-mortem look when you're back. |
Addresses but doesn't completely fix issue #693
The following props were added:
color
: Accepts hex value#FFFFFF
,#000
otherwise a viz palette color will be assignedinitialsLength
: Specify how many characters to show (max 2 allowed). By default, will show based on number of words.initials
: Custom initials (max 2 characters). By default will take the first character (of each word).type
The type of avatar is this displaying ("user" or "space")Some new examples in the docs:
cc @legrego