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 13 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
111 changes: 111 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,114 @@
}
}
}
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--2 {
min-width: 36px;
}

&.AvatarStack--3 {
Copy link
Author

Choose a reason for hiding this comment

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

We should avoid using numerals in class names since they have the potential to be confused with spacer variables.

The two variations for avatar stack component are 2 avatars and ≥3 avatars, right? If so, I'd suggest the following:

  • Default .AvatarStack is for two (with width: 36px)
  • There is a modifier for when a stack has more than two items, called .AvatarStack--more. Since the avatar-more div always needs to be used in stacks with ≥3 items, that could be built into this class. This will lead to cleaner markup, which will reduce the chances of the component being implemented incorrectly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think AvatarStack--more would work, though. Everywhere avatar stacks are currently being used (notifications, projects, commit authors), there is often a single avatar.

If avatar stacks had a 2 avatar minimum, we'd have to implement logic every time we used this component to decide whether to use single avatar markup or a stack (as well as make sure the single avatar had the exact same styles/tooltips/etc to match its surrounding avatar stacks). That feels like a bad tradeoff for cleaner markup to me.

We could possibly use AvatarStack--more and AvatarStack--one for these cases, but making the default 2 avatars feels a bit arbitrary. What do you think?

I don't want to create confusion and I'm very open to using different class names, though. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

@sophshep @califa and I discussed this in slack and we came to the conclusion that AvatarStack--two and AvatarStack-three-plus is the best solution we can think of!

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.

position: absolute;
padding-right: $spacer-1;
background: $bg-white;

.avatar {
position: relative;
z-index: 2;
display: 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: -15px;
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;
margin-right: 0;
border-right: 0;
}

// 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:nth-child(n+4) {
display: inline-block;
opacity: 1;
}

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

.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 {
z-index: 2;
width: 14px;
background: $gray-300;
}

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

.AvatarStack-body:hover .avatar-more {
display: none;
}

// Temp tooltip class to position it correctly

.temp-tooltipped-align-left {
Copy link
Author

Choose a reason for hiding this comment

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

Could we take this out of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I can leave this in the temporary github/github css for now.

&::after {
left: 0 !important;
margin-left: 0 !important;
}

&::before {
left: 5px !important;
}
}
20 changes: 20 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,26 @@ storiesOf('Avatars', module)
</div>

))
.add('AvatarStack', () => (
<div>
<div className='AvatarStack AvatarStack--3' aria-label='five avatars'>
<div className='AvatarStack-body tooltipped tooltipped-se temp-tooltipped-align-left'>
<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--2' aria-label='two avatars'>
<div className='AvatarStack-body tooltipped tooltipped-se temp-tooltipped-align-left'>
<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