New Avatar stack #385

sophshep · 2017-10-27T20:43:31Z

This PR is the start of adding @califa's updated Avatar Stack pattern to Primer.

Here is what I've done so far:

I copied the CSS from @califa's newest PR branch in github/github.
Renamed from .temp-avatar-stack to .AvatarStack so that this is all ready to switch over to our new naming convention. I'm leaving the existing CSS for avatar-stack so that nothing breaks during testing, but eventually we should 🔥 that class.
Added .AvatarStack to Storybook
Refactored the component in isolation and removed any CSS that wasn't necessary (mostly in this commit: f0393d7)

What's next?

Test this style in GitHub using the alpha release
Update docs
Incorporate updates to tooltips so that we don't need the temp-tooltipped-align-left class in here
Delete old avatar-stack from CSS & docs
Add comment to this pr with a brief (one sentence) description of the changes, for inclusion in the Changelog (don't edit the changelog in this pr as it will cause conflicts with v10)

/cc @primer/ds-core @califa

…2a39b0139d9906d1ac2f5b7dbd8c/app/assets/stylesheets/components/avatar-stack.scss

califa · 2017-11-01T15:28:12Z

Hey, I've updated this PR in some ways:

The avatar stack's expanded state is now absolutely positioned as intended on this PR. I think this should be carried over to any other app/area that uses avatar stacks, so I've included it here.
I've added a wrapper around the absolute div so that its initial state interacts with the content around it. This wrapper is now the AvatarStack and the absolute div is AvatarStack-body
I've added classes to tell the container what width it needs to be based on the # of avatars.
- The current naming scheme is AvatarStack--2 and --3 but I'm not married to this.
- These classes need to be added manually (or automatically in the markup).
- The specific widths are based on how the avatar stack appears visually next to adjacent content. They're hard coded, but there's no reason they'll ever have to change.
The background is now set to $bg-white so when expanded you can't see content through it. This can be overridden if needed in individual apps.

The PR I mentioned above updates things on the github/github side. It gets rid of most of the css, and only leaves some app-specific styles.

The team is looking to ship multiple attribution in about two weeks, so it'd be nice to get it into Primer before then 🙌

sophshep

Left some questions & thoughts!

sophshep · 2017-11-01T20:16:12Z

modules/primer-avatars/lib/avatar-stack.scss

+
+// Temp tooltip class to position it correctly
+
+.temp-tooltipped-align-left {


Could we take this out of this PR?

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

sophshep · 2017-11-01T20:20:29Z

modules/primer-avatars/lib/avatar-stack.scss

@@ -43,3 +43,114 @@
    }
  }
 }


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)?

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

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.

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

sophshep · 2017-11-01T20:24:07Z

modules/primer-avatars/lib/avatar-stack.scss

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


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.

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?

@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!

sophshep · 2017-11-01T23:35:14Z

modules/primer-avatars/lib/avatar-stack.scss

+  }
+}
+
+.AvatarStack-body {


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.

@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?

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.

broccolini · 2017-11-05T19:27:51Z

Hey folks, I've been thinking about this over the weekend following Friday's discussion with @sophshep @califa. I thought of a way we can pull this out of v10 without putting pressure on our team to do a v11 release earlier than planned. It occurred to me we can ship the avatar stack update in a follow-up minor release (rather than major) because we have different class names for the new styles. This enables us to add a deprecation warning for the old class names when this is ready to be released, and keep that deprecation warning until we're ready to do a new major release and remove the old styles.

Considering all the risks for trying to include this in v10—between stress some folks are feeling, that there is more work still to be done, and reviews needed—I think we should pull avatar stack updates out of the v10 release, and ship in a follow-up minor release with the deprecation warning.

What does this mean in the context of github/github?

When we pull in the minor release with both the old and new styles, we'll see a deprecation warning when we install, but that won't be good prevention at hubbers using instances of the old styles. We need to look into other methods to warn people not to use soon-to-be-deprecated styles anyway, so we'll make that a priority after v10 ships so that we can hopefully come up with something in time for the avatar stack ship. cc @jonrohan who will likely be the person actually writing the scripts.

Why didn't you suggest this earlier?

Initially my concern was having to rush a v11 release or ship one without anything much in it, if the avatar-stack changes didn't make it into v10. But we can avoid that problem with the deprecation warning. We only recently introduced the deprecation warning which was the first part of figuring out what our process should be—basically we're still learning what works best, what we need to do, and what's possible (with 2 people and limited time). We are also still figuring out a good cadence for releases—this is our first major release after doing a string of releases for the new publishing flow, so we're also still learning what's best for our release process. If it's any consolation, the v10 release is proving to be a great learning experience, so following releases and documentation around process should provide more clarity.

Next steps

Confirm folks are on board with this proposal, cc @califa @sophshep @jonrohan
Confirm timeline for when @califa's team needs this to be shipped (in about 2 weeks I believe), work back from there and prioritize what needs to be done.

I will also be reviewing what else we have on the list for v10 with @jonrohan on Monday, and prioritize what should go into v10 vs a follow-up minor release. I'll write that up and share with everyone currently contributing on Monday for feedback.

califa · 2017-11-06T02:23:29Z

@broccolini Thanks for putting this together! I'm comfortable with this as long as we can definitely hit my team's ship deadline (you're correct about the 2 weeks, I'll get you an exact date tomorrow). I'm free to start working on the changes tomorrow :)

broccolini

Suggested a couple of changes but shouldn't affect you testing on GitHub. Let me know how things go!

broccolini · 2017-11-03T20:47:25Z

modules/primer-avatars/lib/avatar-stack.scss

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


@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!

broccolini · 2017-11-08T20:38:14Z

modules/primer-avatars/lib/avatar-stack.scss

+  }
+}
+
+.AvatarStack-body {


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.

broccolini · 2017-11-08T20:38:58Z

modules/primer-avatars/lib/avatar-stack.scss

+    position: relative;
+    z-index: 2;
+    display: flex;
+    width: 20px;


Sorry I derped! Please leave as is.

broccolini · 2017-11-08T20:42:13Z

modules/primer-avatars/README.md

+
+```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">


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.

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

broccolini · 2017-11-08T20:47:54Z

modules/primer-avatars/README.md

+</div>
+```
+
+If you'd like a right-aligned avatar stack, add the `AvatarStack--right` modifier class and switch the tooltips around:


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.

…ar-stack

Sophie Shepherd added 4 commits October 27, 2017 15:07

Copy CSS over from https://github.com/github/github/blob/8d6e8bcea9f4…

2f1a72f

…2a39b0139d9906d1ac2f5b7dbd8c/app/assets/stylesheets/components/avatar-stack.scss

Rename AvatarStack

dbf4b84

Remove container (for now). And Authors 1&2 css.

20b3186

Add new AvatarStack to storybook

db3ef68

sophshep added status: wip v10 labels Oct 27, 2017

Sophie Shepherd and others added 9 commits October 27, 2017 15:45

lint

e003cd0

Delete a lot of unnecessary (i think?) CSS

f0393d7

cleanup

314b630

fix avatar-more styling

ef42395

implement container and switch classes around

e942b6d

make avatar stacks position absolute, reduce margin

ac78db0

fix linter

6af7e76

add specific sizes for avatar count

bb5bd3b

update storybook

b3d3ebb

sophshep commented Nov 1, 2017

View reviewed changes

califa added 12 commits November 2, 2017 08:08

remove temp tooltip alignment css back to github/github

0f6465d

add first version of non-working right aligned modifier

e374740

refactor right align modifier, update storybook

ff7967f

modify spacing so it works with storybook

a03d4e6

white space workaround

8cca4f5

make ci happy

9c57895

flip padding

80fd1fa

refactor css

251036d

add margins to storybook

a73abae

replace inline-block with flexbox

2fa1d34

re-add last child z-index

ef0f1e5

remove padding now that avatars have margins

da3ae73

sophshep changed the title ~~[WIP] New Avatar stack~~ New Avatar stack Nov 3, 2017

first draft for new avatar stack docs

9e9d9c1

jonrohan added the minor release label Nov 6, 2017

broccolini mentioned this pull request Nov 6, 2017

Tracking issue for Primer 10.1.0 #392

Closed

13 tasks

broccolini added 3 commits November 7, 2017 01:35

move position absolute into container class

7e491e2

replace people avatars with octocats in storybook

03a3d75

update modifier class names

8b99d32

broccolini reviewed Nov 8, 2017

View reviewed changes

broccolini removed the v10 label Nov 8, 2017

califa and others added 5 commits November 9, 2017 11:49

fix length modifiers

4956549

replace employees with octocats, remove redundant alts

c2a392f

Merge branch 'dev' into avatar-stack

8a1074a

Add AvatarStack-body to the readme file

9fec40d

Merge branch 'avatar-stack' of github.com:primer/primer-css into avat…

29f2689

…ar-stack

broccolini changed the base branch from dev to release-10.1.0 November 14, 2017 17:01

broccolini merged commit ef4ac3e into release-10.1.0 Nov 14, 2017

jonrohan added the Tag: Enhancement label Nov 15, 2017

jonrohan deleted the avatar-stack branch June 13, 2018 23:04

This was referenced Dec 16, 2022

Link Checker Report ronna/MERL-Center-public#42

Open

Link Checker Report ronna/MERL-Center-public#44

Open

Link Checker Report ronna/MERL-Center-public#46

Open

This was referenced Dec 27, 2022

Link Checker Report ronna/MERL-Center-public#64

Open

Link Checker Report ronna/MERL-Center-public#72

Open

This was referenced Jan 3, 2023

Link Checker Report ronna/MERL-Center-public#78

Open

Link Checker Report ronna/MERL-Center-public#82

Open

Link Checker Report ronna/MERL-Center-public#86

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Avatar stack #385

New Avatar stack #385

sophshep commented Oct 27, 2017 •

edited by califa

Loading

califa commented Nov 1, 2017

sophshep left a comment

sophshep Nov 1, 2017

califa Nov 2, 2017

sophshep Nov 1, 2017

califa Nov 2, 2017

broccolini Nov 2, 2017

califa Nov 2, 2017

sophshep Nov 1, 2017

califa Nov 2, 2017

broccolini Nov 3, 2017

sophshep Nov 1, 2017

califa Nov 2, 2017

broccolini Nov 8, 2017

broccolini commented Nov 5, 2017

califa commented Nov 6, 2017

broccolini left a comment

broccolini Nov 3, 2017

broccolini Nov 8, 2017

broccolini Nov 8, 2017

broccolini Nov 8, 2017

califa Nov 8, 2017

califa Nov 13, 2017

broccolini Nov 8, 2017

califa Nov 8, 2017


		// Temp tooltip class to position it correctly

		.temp-tooltipped-align-left {

@@ @@ -43,3 +43,114 @@ @@
                   }
                 }
               }

New Avatar stack #385

New Avatar stack #385

Conversation

sophshep commented Oct 27, 2017 • edited by califa Loading

califa commented Nov 1, 2017

sophshep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolini commented Nov 5, 2017

What does this mean in the context of github/github?

Why didn't you suggest this earlier?

Next steps

califa commented Nov 6, 2017

broccolini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophshep commented Oct 27, 2017 •

edited by califa

Loading