Skip to content

Conversation

@shawnbot
Copy link
Contributor

@shawnbot shawnbot commented May 30, 2018

This introduces an <Avatar> component with the following props:

  • alt: string, passed through
  • isChild: boolean, adds the avatar-child class if present
  • size: number; default: 20, adds the avatar-small class if less than 24
  • src: string

Examples:

<Avatar src='/primer.png' />
<Avatar size={40} src='/jonrohan.jpg' alt='jonrohan' />
<Avatar size={20} src='/shawnbot.jpg' isChild />

@shawnbot shawnbot requested a review from a team May 30, 2018 17:50
@jonrohan jonrohan mentioned this pull request May 30, 2018
9 tasks
shawnbot added 3 commits May 30, 2018 16:04
- move query parameter handling to getImageURL() and export that
- rename "child" prop to "isChild" for clarity
@shawnbot
Copy link
Contributor Author

@jonrohan I think I addressed the unnecessary complexity of building the URL inside the component function by moving it into a getImageURL(username, params) function, which is now exported so that consumers can use it when setting src directly. See the tests for examples.

@jonrohan
Copy link
Member

Do we need v=3 in the query string?

I think this is an image cache key that we use for our servers.

<Avatar username='github' size={64} />
</Box>
<Box mb={2}>
<div className='avatar-parent-child float-left'>
Copy link
Contributor Author

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>

Copy link
Contributor Author

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"
Copy link
Contributor Author

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)
Copy link
Contributor Author

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,
Copy link
Contributor Author

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',
Copy link
Contributor Author

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}`}
Copy link
Contributor Author

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.

@shawnbot
Copy link
Contributor Author

This is so much simpler now. 🎉

Flash,
Page,
StateLabel,
Text,
Copy link
Contributor Author

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.

@shawnbot shawnbot changed the base branch from master to release-0.2.0-beta May 31, 2018 21:49
@shawnbot
Copy link
Contributor Author

Okay, this is ready for a final review.

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

Super clear 💯

@shawnbot shawnbot merged commit e9e42e2 into release-0.2.0-beta May 31, 2018
@shawnbot shawnbot deleted the avatars branch May 31, 2018 23:32
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.

4 participants