ui-components: Avatar #48
Conversation
koorosh
requested review from
j-low,
laurenbarker and
nathanstilwell
as code owners
There was a problem hiding this comment.
Looking great so far @koorosh! I'm still working on the TS integration so I couldn't pull this down to test live (since it currently breaks); I may have more feedback at that point but there are a few items in here that can be addressed.
| onClick?: () => void; | ||
| } | ||
|
|
||
| export type AvatarSize = "16" | "24" | "32" | "40"; |
There was a problem hiding this comment.
I'd prefer we use t-shirt sizing; something like: export type AvatarSize = "s" | "m" | "l" | "xl";
There was a problem hiding this comment.
Agree, 👍🏽 the only thing, I would start from "xs" as the smallest one for 16px.
| @@ -0,0 +1,89 @@ | |||
| import React from "react"; | |||
| import { shallow } from "enzyme"; | |||
There was a problem hiding this comment.
@laurenbarker not an issue for this PR, but given the favorable reception of React Testing Lib, would you be onboard if I create an issue to migrate over from Enzyme?
There was a problem hiding this comment.
Was thinking the same thing! Created one this morning #49
| align-items: center; | ||
| justify-content: center; | ||
| flex-shrink: 0; | ||
| font-family: Source Sans Pro, sans-serif; // TODO (koorosh): replace with fonts from tokes |
| @mixin selectable-border($n) { | ||
| box-sizing: border-box; | ||
| border: 2px solid $n; | ||
| } |
There was a problem hiding this comment.
This seems like it would be a good candidate to be added to our tokens, probably with a default color value. It might not be the right time to do so now (i.e. when we have more use cases) but let's add a TODO comment above this to call it out.
Also the let's make var name something descriptive; for example $color... $n has connotations with mathematics as relating to an integer count value so it makes it slightly (though not fatally) confusing.
|
|
||
| .intent-info { | ||
| background: $crl-blue-1; | ||
| color: $crl-blue-3; |
There was a problem hiding this comment.
Wherever possible (or unless there's a specific reason not to do so) let's refer to the base aliases, in this case $crl-base-blue. The reason being the base aliases will be the most common colors seen in the codebase and, conversely, the explicit numerical values should be uncommon enough to call themselves out as a idiosyncratic use case when they do show up.
| } | ||
|
|
||
| &.intent-info { | ||
| @include selectable-border($crl-blue-3); |
There was a problem hiding this comment.
Comment re: base colors from above applies here too.
| } | ||
|
|
||
| &.intent-warning { | ||
| @include selectable-border($crl-yellow-4); |
There was a problem hiding this comment.
Our warning color is $crl-base-yellow/$crl-yellow-3; comment re: base colors from above applies here too.
| } | ||
|
|
||
| &.intent-danger { | ||
| @include selectable-border($crl-red-4); |
There was a problem hiding this comment.
Our warning color is $crl-base-red/$crl-red-3; comment re: base colors from above applies here too.
| Avatar.defaultProps = { | ||
| size: "40", | ||
| intent: "default", | ||
| selectable: false, | ||
| disabled: false, | ||
| }; |
There was a problem hiding this comment.
As far as setting default props, I kind of like this pattern here but I know in other places we've specified them in function signature itself. @laurenbarker do you have an opinion on this at all?
There was a problem hiding this comment.
defaultProps is deprecated for function components (facebook/react#16210) which is why we decided to go with default parameters elsewhere.
koorosh
force-pushed
the
avatar-component
branch
2 times, most recently
from
158a389 to
cb27604
Compare
Fixed! |
nathanstilwell
added
the
✨ candidate-component
| selectable = false, | ||
| onClick, | ||
| }) => { | ||
| const classnames = useMemo( |
There was a problem hiding this comment.
I like the use of useMemo here 👍
|
@koorosh Rachel and I were able to review the avatar component and had some feedback:
|
- Includes base implementation of <Avatar> component which includes ability to modify intent, size, disable, and set selectable props to component. - It is a starting point to extend this implementation for more custom representations as StepAvatar and Validation which require customization of styles. - One more story is added to show <Avatar> component in different states, it is draft version to validate current implementation only.
Avatar component has to be forced to display text with upper-case. Added `transformCase` prop with the same styles as in `Badge` component to follow the same styles.
Avatar component sizing is changed to have only two options: default and small sizes. Very small sizes are removed (xs, and s sizes) Also size naming are changed to reflect that only two options are available for now.
@Annebirzin , thanks for your feedback!
|
|
thanks @koorosh! |
ui-components // Avatar
Issue #22
the ability to modify intent, size, disable, and set selectable props
to component.
custom representations as StepAvatar and Validation which require
customization of styles.
states, it is a draft version to validate current implementation only.
Checklist