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

Jump to bottom

feat!: rework Avatar, User and UserLabel components #1991

Merged
merged 1 commit into from
Dec 26, 2024
Merged

feat!: rework Avatar, User and UserLabel components #1991

merged 1 commit into from
Dec 26, 2024

Conversation

DakEnviy
Copy link
Contributor

@DakEnviy DakEnviy commented Dec 12, 2024 •
edited
Loading

Ticket: DATAUI-2913

Breaking changes

Avatar

  • One of the following properties is required: imgUrl, icon or text
  • Change the size of the text for s size
  • Change the border width for 2xs size
  • Rename CSS variable --g-avatar-color to --g-avatar-text-color

User

  • Change the gap between the avatar and text for m size
  • Change font size for xl size

UserLabel

  • Replace children property with text
  • Change all gaps and font sizes
  • Add a border for avatar

Features

Avatar

  • Add 3xs size
  • Support all AriaLabelingProps
  • Add --g-avatar-border-width, --g-avatar-inner-border-width and --g-avatar-font-weight to CSS API

User

  • Add 3xs size
  • Support all AriaLabelingProps
  • Rework CSS API (replace all the variables with new ones)
  • Support string as an avatar property

UserLabel

  • Add 3xs and 2xs sizes
  • Add description property
  • Rework CSS API (replace all the variables with new ones)

@DakEnviy DakEnviy requested a review from amje December 12, 2024 20:28
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

amje
amje requested changes Dec 13, 2024
View reviewed changes
src/components/Avatar/types/main.ts Outdated Show resolved Hide resolved
src/components/User/README.md Show resolved Hide resolved
src/components/User/index.ts Show resolved Hide resolved
@DakEnviy DakEnviy force-pushed the feat/new-avatar-sizes branch from 9c488e7 to 4c23e0b Compare December 15, 2024 12:43
@DakEnviy DakEnviy changed the title WIP: rework user components and add 3xs size feat!: rework Avatar, User and UserLabel components Dec 15, 2024
@DakEnviy DakEnviy force-pushed the feat/new-avatar-sizes branch 2 times, most recently from 387a2ec to b6b7654 Compare December 17, 2024 10:34
@DakEnviy DakEnviy requested a review from amje December 17, 2024 15:17
@DakEnviy DakEnviy force-pushed the feat/new-avatar-sizes branch from b6b7654 to c2dec6b Compare December 17, 2024 17:41
korvin89
korvin89 reviewed Dec 18, 2024
View reviewed changes
src/components/Avatar/Avatar.scss
&__icon {
color: var(--g-avatar-color, var(--_--color));

& > svg {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need it? Looks unreliable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to center svg element in the icon properly

src/components/Avatar/_variables.scss
@@ -1,6 +1,7 @@
@use 'sass:map';

$sizes: (
'3xs': 16px,
'2xs': 20px,
'xs': 24px,
's': 28px,
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that we wanted to change this sizes like in Button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to make in the next major release

amje
amje reviewed Dec 18, 2024
View reviewed changes
src/components/User/README.md Outdated
| `--g-user-line-height` | Name and description line height |
| Name | Description |
| :--------------------------------- | :-------------------------------- |
| `--g-user-gap` | Gap between avatar and text block |
Copy link
Contributor

Choose a reason for hiding this comment

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

What about --g-user-avatar-offset name for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXED

@DakEnviy DakEnviy force-pushed the feat/new-avatar-sizes branch from c2dec6b to 33876a5 Compare December 26, 2024 12:19
amje
amje requested changes Dec 26, 2024
View reviewed changes
src/components/Avatar/README.md Outdated
| `--g-avatar-inner-border-width` | Inner border width |
| `--g-avatar-border-color` | Border color |
| `--g-avatar-background-color` | Background color |
| `--g-avatar-color` | Icon and text color |
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name it --g-avatar-text-color similar to other components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just color because this color is used for icon and text both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXED

src/components/UserLabel/README.md
Comment on lines +111 to +113
| `--g-user-label-text-font-weight` | Text font weight |
| `--g-user-label-text-font-size` | Text font size |
| `--g-user-label-text-line-height` | Text line height |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why text and not name like in User?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use text because the property in the component is named text too. The property is called so because it can be email also.

src/components/UserLabel/README.md Outdated
Comment on lines 109 to 110
| `--g-user-label-outer-gap` | Label horizontal padding |
| `--g-user-label-inner-gap` | Gap between elements (avatar, text, close icon) |
Copy link
Contributor

Choose a reason for hiding this comment

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

These names are not meaningful enough. Can we improve them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to --g-user-label-padding and --g-user-label-gap

@DakEnviy DakEnviy force-pushed the feat/new-avatar-sizes branch from 33876a5 to 077aded Compare December 26, 2024 14:12
@DakEnviy DakEnviy force-pushed the feat/new-avatar-sizes branch from 077aded to 150c867 Compare December 26, 2024 14:33
@DakEnviy DakEnviy merged commit 515f4d5 into next Dec 26, 2024
6 checks passed
@DakEnviy DakEnviy deleted the feat/new-avatar-sizes branch December 26, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@korvin89 korvin89 korvin89 approved these changes

@amje amje amje approved these changes

@ValeraS ValeraS Awaiting requested review from ValeraS ValeraS is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DakEnviy @gravity-ui-bot @amje @korvin89