-
Notifications
You must be signed in to change notification settings - Fork 45
feat(avatar-stack): new component #4331
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 2e8db38 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Preview deployments for this pull request: storybook - |
|
Needs feedback/approval before creating documentation on www |
packages/css/src/avatar-stack.css
Outdated
| } | ||
|
|
||
| .ds-avatar-stack { | ||
| /*The following variables can be defined in a parent of .ds-avatar-stack: |
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.
out of curiosity, why do we want these variables to potentially be defined in the parent? And if so, should we now start the work of making this possible for all components? 🤔
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.
This is how i would have implemented the public variables so they can be inherited instead of having to override the class.
--gap: var(--dsc-avatar-stack-gap, 2);
a short-named local variable pointing to the undefined full-named variable --dsc-avatar-stack-gap or otherwise use the fallback
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 see
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 think we should stick to our convention here. Our consumers expect our variables to be prefixed with --ds- or --dsc-, and having variables such as --gap goes against this, and opens up for unwanted behaviour
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.
The --dsc- variables are there just as in every component, the only difference being they can be defined on root or in a parent and inherited down for a marginal extra convenience. But i'll remove this feature for now 👍
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.
yes, very much want this convenience <3 but let's do it will all components at once so we find and test edge cases and potential solutions at the same time
| gap, | ||
| avatarSize, | ||
| overlap, | ||
| max = 4, |
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'm unsure if max should be part of the component, or if we should rather have:
<AvatarStack>
<Avatar />
<Avatar />
+2
</AvatarStack>
This would make the CSS and component less complex, and closer to the pure HTML/CSS edition?
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.
Perhaps.. Since we cant use sibling-count() yet there is javascript required for this logic regardless. 🤷
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.
mm, but, why would we need sibling-count? I'm, thinking it is the consumers responsibility to add the correct amount of Avatar-components in the element?
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 agree, it's fine to make the consumer add the "remaining" number themselves.
A data attr should be fine here, if that's easier for the CSS to work.
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.
(logic involving sibling-count could possibly be used to automatically display: none the beyond maximum avatars i think.. but it is not relevant for a while anyway)
I can delete the react logic but keep the data-additionals selector perhaps, for easier styling, instead of putting text-fragment as a child?
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.
Sound good to me :)
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.
display:none logic - what is in the HTML is what the user sees 🤝 😇
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 added align-items: center so now you can choose to use data-suffix for pre-styled text or add whatever you want as a last child inside AvatarStack. Unfortunately adding something as first-child works less well because there is no :nth-first-child(1 of .ds-avatar) as there is :nth-last-child(1 of .ds-avatar), only :first-child (needed to exclude negative margin in the first 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.
Does :first-child:is(.ds-avatar) work?
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 see, hm, how about .ds-avatar:first-child for the first, and .ds-avatar:not(:has(+ .ds-avatar)) for the last?
|
|
||
| .ds-avatar-stack { | ||
| --dsc-avatar-stack-size: var(--ds-size-12); | ||
| --dsc-avatar-stack-gap: 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.
Suggestion: can we specify unit here?
| --dsc-avatar-stack-gap: 2; | |
| --dsc-avatar-stack-gap: 2px; |
as this gives more flexibility?
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.
Do you feel like the react prop should then specify unit as well or keep that as number converted to px?
| .ds-avatar-stack { | ||
| --dsc-avatar-stack-size: var(--ds-size-12); | ||
| --dsc-avatar-stack-gap: 2; | ||
| --dsc-avatar-stack-overlap: 50; |
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.
Suggestion: can we specify unit here too?
| --dsc-avatar-stack-overlap: 50; | |
| --dsc-avatar-stack-overlap: 50px; |
It seems like this would not be problematic, as the rest of the equations are +/-
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.
This needs to be a unitless number. it is used in a calculation to create the overlap as a percentage of the avatar size automatically
| --_gap: calc(var(--dsc-avatar-stack-gap) * 1px); | ||
| --_overlap: calc(((var(--dsc-avatar-stack-size) / 100) * var(--dsc-avatar-stack-overlap)) * -1); | ||
|
|
||
| --captured-length: var(--_overlap); |
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.
could we skip this, a just use --_overlap directly in the rest of the file?
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.
The registered property is needed to be able to animate the optional expand action. We could rename that to --_overlap or something else instead, but since they have to be declared in global scope, I am in favour of generically named reusable registered custom properties
| .ds-avatar { | ||
| --dsc-avatar-size: var(--dsc-avatar-stack-size); | ||
| --_font-size: max(0.75rem, calc(var(--dsc-avatar-stack-size) * 0.5)); | ||
| mask: radial-gradient(50% 50% at calc(150% + var(--captured-length)), transparent calc(100% - 1px + var(--_gap)), #000 calc(100% + var(--_gap))); |
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.
very clever <3
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 don't know who came up with this technique first, but it was not me!
| font-size: var(--_font-size); | ||
| } | ||
| > a { | ||
| line-height: 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.
I do not understand these child selectors (&[data-initials]:empty::before, > span and > a) intuitively - why are they needed? Why are they not in .ds-avatar? I assume there is good reasons, so a comment would be super
| &:not(:first-child) { | ||
| margin-left: var(--captured-length); | ||
| } | ||
| &:nth-last-child(1 of .ds-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.
Could this instead be .ds-avatar:not(:has(+ .ds-avatar)) for even better browser support?
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.
looks like :has came out a year earlier than :nth-last-child(1 of ...) for chrome (2022 vs 2023), while :has came after nth-last-child in the other browsers. I can add @supports blocks and use both i suppose
https://caniuse.com/css-nth-child-of
https://caniuse.com/css-has
| > | ||
| <legend>Link expandable</legend> | ||
| <AvatarStack {...args} expandable='fixed'> | ||
| <Avatar aria-label=''> |
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.
Can we use asChild on avatar, so we avoid extra nesting?
| <Avatar aria-label=''> | |
| <Avatar aria-label='' asChild> |
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.
are you thinking asChild="a" use case? It would be a new feature added to Avatar
resolves #2306