-
Notifications
You must be signed in to change notification settings - Fork 646
Add Avatar component #44
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
Conversation
- move query parameter handling to getImageURL() and export that - rename "child" prop to "isChild" for clarity
|
@jonrohan I think I addressed the unnecessary complexity of building the URL inside the component function by moving it into a |
I think this is an image cache key that we use for our servers. |
examples/index.js
Outdated
| <Avatar username='github' size={64} /> | ||
| </Box> | ||
| <Box mb={2}> | ||
| <div className='avatar-parent-child float-left'> |
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.
Note: I haven't yet implemented the parent-child component, but I was thinking of something like this doing it:
<Avatar username='github' size={48}>
<Avatar username='primer' size={20} isChild />
</Avatar>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.
Let's hold off on this. 😁
package.json
Outdated
| "react": "^16.2.0", | ||
| "system-classnames": "^1.0.0-3" | ||
| "system-classnames": "^1.0.0-3", | ||
| "url-search-params": "^0.10.0" |
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.
Note: I needed the URLSearchParams ponyfill so that we can render the docs statically (i.e. outside of the browser). I'm honestly not sure how the test suite was running successfully without this. 🤔
src/Avatar.js
Outdated
|
|
||
| const { | ||
| alt = username, | ||
| src = getImageURL(username, params) |
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.
We agreed that this is mixing concerns. The Avatar shouldn't be calculating URLs and should just take src directly. We might eventually need, for instance, a GitHubAvatar that does something like:
function GitHubAvatar({username = 'github', ...rest}) {
return <Avatar {...rest} src={getUsernameURL(username)} />
}
src/Avatar.js
Outdated
| const Avatar = props => { | ||
| const { | ||
| username = 'github', | ||
| size = 20, |
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.
We could limit this to a set of named sizes (small, medium, large), or have a way of mapping those to pixel sizes.
src/Avatar.js
Outdated
|
|
||
| const Avatar = props => { | ||
| const { | ||
| username = 'github', |
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.
TODO: pass alt through as-is.
|
|
||
| const GitHubAvatar = ({username, size = 20, ...rest}) => ( | ||
| <Avatar | ||
| src={`https://avatars.githubusercontent.com/${username}?v=3&s=${size * 2}`} |
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.
I've implemented this in our examples so that we can easily change avatars and sizes.
|
This is so much simpler now. 🎉 |
| Flash, | ||
| Page, | ||
| StateLabel, | ||
| Text, |
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.
Note that I've sorted these imports alphabetically.
|
Okay, this is ready for a final review. |
jonrohan
left a comment
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.
Super clear 💯
This introduces an
<Avatar>component with the following props:alt: string, passed throughisChild: boolean, adds theavatar-childclass if presentsize: number; default: 20, adds theavatar-smallclass if less than 24src: stringExamples: