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.

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

#373 Avatar component #375

Merged
merged 7 commits into from
Aug 5, 2022
Merged

#373 Avatar component #375

merged 7 commits into from
Aug 5, 2022

Conversation

sgraczyk
Copy link

@sgraczyk sgraczyk commented Aug 1, 2022

Resolves: #373

Description

Implements the Avatar component from scratch. During the development phase I made some decisions to be confirmed by the rest of the team:

  • I decided to make the API more generic. For example instead of kind: 'person' | 'group' I've used shape: 'circle' : 'rounded-square'. A similar change was for the status, as not all products are chat-focused, so I avoided the accepts chats name.
  • Figma project does not specify the behavior of rim in case of rounded-square, nevertheless, I implemented it.
  • I'm a bit confused on how to address the position of the status icon as the design seems to do it differently for each size and square. I tried to follow that assumption with some slight modifications.

Storybook

Checklist

Obligatory:

  • Self-review
  • Unit & integration tests
  • Storybook cases
  • Design review by @kstvsko
  • Functional (QA) review by @TomaszRatajczyk

Optional:

  • Accessibility cases (keyboard control, correct HTML markup, etc.)

@@ -0,0 +1,16 @@
import { getContrast } from 'polished';

export function getInitials(name = '', count = 2): string {
Copy link
Author

Choose a reason for hiding this comment

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

At this moment it does not support diacritics or emojis.

@sgraczyk sgraczyk linked an issue Aug 3, 2022 that may be closed by this pull request
@sgraczyk sgraczyk marked this pull request as ready for review August 3, 2022 10:36
@kstvsko
Copy link

kstvsko commented Aug 3, 2022

@sgraczyk

I've added updated avatars, also with status indicators, that are now scaled and positioned at the same place at every size. I've added also the scale for the avatars with rims. :)

Rims: https://www.figma.com/file/2pFu80PXO5A2tfyrAGnx91/Product-Components?node-id=8167%3A31183
Status Indicators: https://www.figma.com/file/2pFu80PXO5A2tfyrAGnx91/Product-Components?node-id=736%3A8527

@MichalPaszowski
Copy link
Contributor

Something not pykło here https://monosnap.com/file/1aOi1ZZ2lR00rwnWGF2JmDl9Ks13r3

@sgraczyk
Copy link
Author

sgraczyk commented Aug 4, 2022

Something not pykło here https://monosnap.com/file/1aOi1ZZ2lR00rwnWGF2JmDl9Ks13r3

Should be fixed within 99b29a1

@sgraczyk
Copy link
Author

sgraczyk commented Aug 4, 2022

@sgraczyk

I've added updated avatars, also with status indicators, that are now scaled and positioned at the same place at every size. I've added also the scale for the avatars with rims. :)

Rims: https://www.figma.com/file/2pFu80PXO5A2tfyrAGnx91/Product-Components?node-id=8167%3A31183 Status Indicators: https://www.figma.com/file/2pFu80PXO5A2tfyrAGnx91/Product-Components?node-id=736%3A8527

Status indicators are implemented as proposed, however, I'd like to wait before introducing changes designed for rims. It looks that now the size of the image is adjusted so the rim fits into the original size of the avatar. It'll probably introduce a flickering effect, especially that the rim is a UI element that may appear and disappear when a specific condition changes. Can we move the work around rims to another issue, and accept the current state for now?

@sgraczyk sgraczyk requested a review from a team August 4, 2022 15:19
@sgraczyk sgraczyk merged commit 39bd267 into v1 Aug 5, 2022
@sgraczyk sgraczyk deleted the feature/373 branch August 5, 2022 08:09
'xxxsmall',
'xxsmall',
'xsmall',
];
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space - perhaps it should be put up top of the file?

'xlarge',
'xxlarge',
];
it.each(smallAndAboveSizes)(
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

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.

[Avatar] Component
5 participants