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

fix(Avatar): keepAspectRation=true by default #7838

Closed
wants to merge 3 commits into from
Closed

fix(Avatar): keepAspectRation=true by default #7838

wants to merge 3 commits into from

Conversation

EldarMuhamethanov
Copy link
Contributor

@EldarMuhamethanov EldarMuhamethanov commented Oct 25, 2024 •
edited
Loading

  • Release notes

Описание

Нужно для компонента Avatar сделать keepAspectRation=true по дефолту

Изменения

Сделал keepAspectRation=true для Avatar по дефолту.
Заметил, что keepAspectRatio работает неправильно в Avatar, когда не задан widthSize и heightSize и доработал этот момент
Codmod как мне кажется не нужен, так как аватары итак обычно одинаковые по ширине и высоте

Release notes

Улучшения

  • Avatar: cвойство keepAspectRation теперь true by default

@EldarMuhamethanov EldarMuhamethanov added this to the v7.0.0-beta.2 milestone Oct 25, 2024
@EldarMuhamethanov EldarMuhamethanov self-assigned this Oct 25, 2024
@EldarMuhamethanov EldarMuhamethanov requested a review from a team as a code owner October 25, 2024 08:10
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 25, 2024 •
edited
Loading

size-limit report 📦

Path Size
JS 381.09 KB (+0.03% 🔺)
JS (gzip) 115.5 KB (+0.05% 🔺)
JS (brotli) 95.03 KB (-0.03% 🔽)
JS import Div (tree shaking) 1.46 KB (0%)
CSS 332.24 KB (0%)
CSS (gzip) 42.12 KB (0%)
CSS (brotli) 33.21 KB (0%)

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Oct 25, 2024 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 25, 2024 •
edited
Loading

e2e tests

Playwright Report

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 25, 2024 •
edited
Loading

👀 Docs deployed

Commit fc089c2

@codecov Codecov
Copy link

codecov bot commented Oct 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.16%. Comparing base (e6ce861) to head (fc089c2).
Report is 23 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7838      +/-   ##
==========================================
+ Coverage   95.14%   95.16%   +0.01%     
==========================================
  Files         377      377              
  Lines       11092    11096       +4     
  Branches     3687     3691       +4     
==========================================
+ Hits        10554    10560       +6     
+ Misses        538      536       -2
Flag Coverage Δ
unittests 95.16% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EldarMuhamethanov EldarMuhamethanov marked this pull request as draft October 25, 2024 08:17
@EldarMuhamethanov EldarMuhamethanov marked this pull request as ready for review October 25, 2024 09:27
BlackySoul
BlackySoul reviewed Oct 28, 2024
View reviewed changes
packages/vkui/src/components/Avatar/Avatar.tsx
@@ -87,6 +87,7 @@ export const Avatar: React.FC<AvatarProps> & {
initials,
fallbackIcon: fallbackIconProp,
children,
keepAspectRatio = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Мы такое поведение правили 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

прикольно, но я думаю кейс с аватаркой у которой соотношение сторон может быть разным - очень редкий кейс

Copy link
Contributor

Choose a reason for hiding this comment

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

Тогда можно переименовать в disableAspectRatio или что-то вроде того. Я думаю, что Вика это имела ввиду.

Copy link
Contributor

@inomdzhon inomdzhon Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Тогда можно переименовать в disableAspectRatio или что-то вроде того. Я думаю, что Вика это имела ввиду.

Как вариант

@qurle напомни, почему в целом хочется сделать true?

Copy link
Contributor

Choose a reason for hiding this comment

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

@qurle напомни, почему в целом хочется сделать true?

Мне кажется, что я мог неправильно понять назначение пропа, приду в личку.

@EldarMuhamethanov
Copy link
Contributor Author

Обсудили и решили, что эта доработка не нужна

@qurle
Copy link
Contributor

qurle commented Oct 29, 2024

Простите братья

@inomdzhon inomdzhon deleted the e.muhamethanov/avatar-keepAspectRatio-by-default branch October 30, 2024 09:12
@inomdzhon inomdzhon removed this from the v7.0.0-beta.2 milestone Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrey-medvedev-vk andrey-medvedev-vk andrey-medvedev-vk left review comments

@qurle qurle qurle left review comments

@BlackySoul BlackySoul BlackySoul left review comments

@inomdzhon inomdzhon inomdzhon left review comments

Assignees

@EldarMuhamethanov EldarMuhamethanov

Labels
cmp:avatar
Projects
VKUI
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EldarMuhamethanov @qurle @inomdzhon @BlackySoul @andrey-medvedev-vk