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.

Sign up for GitHub

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

New Avatar stack #385

Merged
merged 35 commits into from
Nov 14, 2017
Merged

New Avatar stack #385

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
2f1a72f
Copy CSS over from https://github.com/github/github/blob/8d6e8bcea9f4…
Oct 27, 2017
dbf4b84
Rename AvatarStack
Oct 27, 2017
20b3186
Remove container (for now). And Authors 1&2 css.
Oct 27, 2017
db3ef68
Add new AvatarStack to storybook
Oct 27, 2017
e003cd0
lint
Oct 27, 2017
f0393d7
Delete a lot of unnecessary (i think?) CSS
Oct 27, 2017
314b630
cleanup
Oct 27, 2017
ef42395
fix avatar-more styling
califa Oct 31, 2017
e942b6d
implement container and switch classes around
califa Oct 31, 2017
ac78db0
make avatar stacks position absolute, reduce margin
califa Oct 31, 2017
6af7e76
fix linter
califa Nov 1, 2017
bb5bd3b
add specific sizes for avatar count
califa Nov 1, 2017
b3d3ebb
update storybook
califa Nov 1, 2017
0f6465d
remove temp tooltip alignment css back to github/github
califa Nov 2, 2017
e374740
add first version of non-working right aligned modifier
califa Nov 2, 2017
ff7967f
refactor right align modifier, update storybook
califa Nov 2, 2017
a03d4e6
modify spacing so it works with storybook
califa Nov 2, 2017
8cca4f5
white space workaround
califa Nov 2, 2017
9c57895
make ci happy
califa Nov 2, 2017
80fd1fa
flip padding
califa Nov 2, 2017
251036d
refactor css
califa Nov 2, 2017
a73abae
add margins to storybook
califa Nov 2, 2017
2fa1d34
replace inline-block with flexbox
califa Nov 2, 2017
ef0f1e5
re-add last child z-index
califa Nov 3, 2017
da3ae73
remove padding now that avatars have margins
califa Nov 3, 2017
6ab6594
remove unrequired css
califa Nov 3, 2017
9e9d9c1
first draft for new avatar stack docs
califa Nov 6, 2017
7e491e2
move position absolute into container class
broccolini Nov 7, 2017
03a3d75
replace people avatars with octocats in storybook
broccolini Nov 7, 2017
8b99d32
update modifier class names
broccolini Nov 7, 2017
4956549
fix length modifiers
califa Nov 9, 2017
c2a392f
replace employees with octocats, remove redundant alts
califa Nov 13, 2017
8a1074a
Merge branch 'dev' into avatar-stack
jonrohan Nov 13, 2017
9fec40d
Add AvatarStack-body to the readme file
califa Nov 13, 2017
29f2689
Merge branch 'avatar-stack' of github.com:primer/primer-css into avat…
califa Nov 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 46 additions & 6 deletions modules/primer-avatars/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,54 @@ When you need a larger parent avatar, and a smaller child one, overlaid slightly

### Avatar stack

Stacked avatars can be used to show who is participating in thread when there is limited space available. When you hover over the stack, the avatars will reveal themselves. Optimally, you should put no more than 3 avatars in the stack.
Stacked avatars can be used to show multiple collaborators or participants when there is limited space available. When you hover over the stack, the avatars will reveal themselves.

```html
<span class="avatar-stack tooltipped tooltipped-s" aria-label="jonrohan, aaronshekey, and josh">
<img alt="@jonrohan" class="avatar" height="39" alt="jonrohan" src="/jonrohan.png" width="39">
<img alt="@aaronshekey" class="avatar" height="39" alt="aaronshekey" src="/aaronshekey.png" width="39">
<img alt="@josh" class="avatar" height="39" alt="josh" src="/josh.png" width="39">
</span>
<div class="AvatarStack AvatarStack--three-plus tooltipped tooltipped-se temp-tooltipped-align-left" aria-label="jonrohan, aaronshekey, and josh">
<img alt="@jonrohan" class="avatar" height="20" alt="jonrohan" src="/jonrohan.png" width="20">
<img alt="@aaronshekey" class="avatar" height="20" alt="aaronshekey" src="/aaronshekey.png" width="20">
<img alt="@josh" class="avatar" height="20" alt="josh" src="/josh.png" width="20">
</div>
```

Based on the number of avatars in the stack, add these modifier classes:
- `AvatarStack--two` for 2 avatars.
- `AvatarStack--three-plus` for 3 or more avatars.

If you have more than three avatars, add a div with the classes `avatar avatar-more` as the third avatar in the stack, as such:

```html
<div class="AvatarStack AvatarStack--three-plus tooltipped tooltipped-se temp-tooltipped-align-left" aria-label="jonrohan, aaronshekey, and josh">
<img alt="@jonrohan" class="avatar" height="20" alt="jonrohan" src="/jonrohan.png" width="20">
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in Slack, we're removing staff avatars from our docs and moving to using the octocat placeholder. Could you update to use this? https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png - let me know if you have trouble or don't have time.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@broccolini should I replace people w/ octocts for this entire readme file?

<img alt="@aaronshekey" class="avatar" height="20" alt="aaronshekey" src="/aaronshekey.png" width="20">
<div class="avatar avatar-more"></div>
<img alt="@josh" class="avatar" height="20" alt="josh" src="/josh.png" width="20">
<img alt="@josh" class="avatar" height="20" alt="josh" src="/josh.png" width="20">
<img alt="@josh" class="avatar" height="20" alt="josh" src="/josh.png" width="20">
</div>
```

You can also link individual avatars. To do this shift the `avatar` class over to the anchor:

```html
<div class="AvatarStack AvatarStack--two tooltipped tooltipped-se temp-tooltipped-align-left" aria-label="jonrohan, aaronshekey, and josh">
<a href="#" class="avatar">
<img alt="@jonrohan" height="20" alt="jonrohan" src="/jonrohan.png" width="20">
</a>
<a href="#" class="avatar">
<img alt="@josh" height="20" alt="josh" src="/josh.png" width="20">
</a>
</div>
```

If you'd like a right-aligned avatar stack, add the `AvatarStack--right` modifier class and switch the tooltips around:
Copy link
Member

Choose a reason for hiding this comment

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

Trying to make our docs a bit more "to the point" and more of a command tense. Suggest:

Use AvatarStack--right to right-align the avatar stack and switch the origin of the tooltips.

"and switch the tooltips around" - it doesn't switch it though right? You have to manually change the tooltip classes? If so, we should make that clear. Suggest:

Use AvatarStack--right to right-align the avatar stack. Remember to switch the alignment of tooltips when making this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


```html
<div class="AvatarStack AvatarStack--three-plus AvatarStack--right tooltipped tooltipped-sw temp-tooltipped-align-right" aria-label="jonrohan, aaronshekey, and josh">
<img alt="@jonrohan" class="avatar" height="20" alt="jonrohan" src="/jonrohan.png" width="20">
<img alt="@aaronshekey" class="avatar" height="20" alt="aaronshekey" src="/aaronshekey.png" width="20">
<img alt="@josh" class="avatar" height="20" alt="josh" src="/josh.png" width="20">
</div>
```

## Circle Badge
Expand Down
134 changes: 134 additions & 0 deletions modules/primer-avatars/lib/avatar-stack.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,137 @@
}
}
}
Copy link
Author

Choose a reason for hiding this comment

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

I left avatar-stack in here when I started the PR to prevent breaking anything, but ideally we could take it out of Primer in favor of the refactored/redesigned component. Could we take it out here (and in replace it in github/github)?

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely could (and should!), but I don't think that's feasible by tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @califa we need to update all the instances of avatar in GitHub for this to ship. As I understand it, this pattern is to replace the current avatar stack, which means we need to ensure this pattern works everywhere it's being used. The best way to test this is to update the locations where this is being used. If you can't do that in time for v10 release, or you need help with that, then we'll need to discuss whether to pull it out of v10 or delay it by a few days. I'd prefer to get it into v10 because I think these changes will require a major release and I don't really want to do v11 straight after v10.

For clarity on timeline, we want all v10 pr's merged in eod tomorrow, so that we can do a naming change on Monday and test, and then intend to ship Monday or Tuesday depending on how long it takes to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

@broccolini 👍 talked to @sophshep. I'm spending all day on this today.


// Refactored, new avatar stack:

.AvatarStack {
position: relative;
min-width: 26px;
height: 20px;

.AvatarStack-body {
position: absolute;
}

&.AvatarStack-two {
min-width: 36px;
}

&.AvatarStack-three-plus {
min-width: 46px;
}
}

.AvatarStack-body {
Copy link
Author

Choose a reason for hiding this comment

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

I'm curious to have @broccolini weigh in here about .AvatarStack as the containing div that .AvatarStack-body is positioned absolutely within.

I spoke to @califa briefly on slack about this and he said that this component will always need to be positioned this way in order to interact with its surroundings as intended. This does, however, go against the principle of "separate container and content" and has the potential to lead to some unnecessary overrides in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sophshep I think this might actually align with that principle.

Separate container and content: Essentially, this means “rarely use location-dependent styles”. A component should look the same no matter where you put it.

If we treat the container as part of the component as we're doing here, then it won't be location dependent. It'll always appear/behave exactly the same no matter where we put it.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I've updated AvatarStack to apply position: absolute; to AvatarStack-body so that it partially addresses mine and @sophshep's concern's (above) re tying the positioning into this component (but not entirely since you have width in the block class).

@califa - might be good if we have a longer discussion in person about our principles, I'd like to ensure they make sense to everyone and I understand how they might be interpreted differently. We haven't really updated them since we first started the design systems team, and I want to review and iterate on them, and ensure they tie back to product design principles so that we can use them as a framework to make decisions on both design and code more easily.

To be honest I expect we'll need to iterate on AvatarStack(like many things). We want to move to more consistent usage around width and height values so they would point to variables or use utilities, have more layout objects (incl. starting to use CSS grid), and I think we'll have other components that will require this same "stack" treatment than just avatars. But I'm fine with this shipping in the interest of having this for your feature, and it will give us an opportunity to test the new version out.

display: flex;
background: $bg-white;

.avatar {
position: relative;
z-index: 2;
display: flex;
Copy link
Author

Choose a reason for hiding this comment

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

What does this declaration do? (Things seem fine without it)

Copy link
Contributor

Choose a reason for hiding this comment

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

This gets rid of the weird white-space disparity caused by inline-block :)

width: 20px;
Copy link
Member

Choose a reason for hiding this comment

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

Use $spacer-4 for the width and height instead of static values.

Copy link
Contributor

Choose a reason for hiding this comment

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

$spacer-4 is 24px. these need to be 20px to work in all the various views across the app.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I derped! Please leave as is.

height: 20px;
box-sizing: content-box;
margin-right: -11px;
background-color: $bg-white;
border-right: $border-width $border-style $white;
border-radius: 2px;
transition: margin 0.1s ease-in-out;

&:first-child {
z-index: 3;
}

&:last-child {
z-index: 1;
}

// stylelint-disable selector-max-type
img {
border-radius: 2px;
}
// stylelint-enable selector-max-type

// Account for 4+ avatars
&:nth-child(n+4) {
display: none;
opacity: 0;
}
}

&:hover {
.avatar {
margin-right: 3px;
}

.avatar:nth-child(n+4) {
display: flex;
opacity: 1;
}

.avatar-more {
display: none !important;
}
}
}

.avatar.avatar-more {
z-index: 1;
margin-right: 0;
background: $gray-100;

&::before,
&::after {
position: absolute;
display: block;
height: 20px;
content: "";
border-radius: 2px;
outline: $border-width $border-style $white;
}

&::before {
width: 17px;
background: $gray-200;
}

&::after {
width: 14px;
background: $gray-300;
}
}

// Right aligned variation

.AvatarStack--right {
.AvatarStack-body {
right: 0;
flex-direction: row-reverse;
Copy link
Author

Choose a reason for hiding this comment

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

Niiiiice


&:hover .avatar {
margin-right: 0;
margin-left: 3px;
}
}

.avatar.avatar-more {
background: $gray-300;

&::before {
width: 5px;
}

&::after {
width: 2px;
background: $gray-100;
}
}

.avatar {
margin-right: 0;
margin-left: -11px;
border-right: 0;
border-left: $border-width $border-style $white;
}
}
37 changes: 37 additions & 0 deletions modules/primer-avatars/stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,43 @@ storiesOf('Avatars', module)
</div>

))
.add('AvatarStack', () => (
<div>
<div className='AvatarStack AvatarStack-three-plus mb-2'>
<div className='AvatarStack-body tooltipped tooltipped-se temp-tooltipped-align-left' aria-label='five avatars'>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<div className='avatar-more avatar'></div>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
</div>
</div>
<div className='AvatarStack AvatarStack-two mb-2'>
<div className='AvatarStack-body tooltipped tooltipped-se temp-tooltipped-align-left' aria-label='two avatars'>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
</div>
</div>
<div className='AvatarStack AvatarStack-three-plus AvatarStack--right mb-2'>
<div className='AvatarStack-body tooltipped tooltipped-sw temp-tooltipped-align-right' aria-label='five avatars'>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<div className='avatar-more avatar'></div>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
</div>
</div>
<div className='AvatarStack AvatarStack-two AvatarStack--right mb-2'>
<div className='AvatarStack-body tooltipped tooltipped-sw temp-tooltipped-align-right' aria-label='two avatars'>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
<img className='avatar' alt='Uncle Cat' width='20' height='20' src='https://user-images.githubusercontent.com/334891/29999089-2837c968-9009-11e7-92c1-6a7540a594d5.png'/>
</div>
</div>

</div>
))
.add('CircleBadge--small', () => (
<div>
<a className='CircleBadge CircleBadge--small' href='#url' title='Travis CI'>
Expand Down