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

feat(Cell/SimpleCell/MiniInfoCell/RichCell): Use spacing size tokens #7033

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mendrew
Copy link
Contributor

@mendrew mendrew commented Jun 19, 2024


  • Unit-тесты
  • e2e-тесты
  • Дизайн-ревью

Описание

Заменяем константы отступов на токены системы расстояний.

Токены системы расстояний из vkui-tokens:
https://github.com/VKCOM/vkui-tokens/blob/2222b5b2208df9ea2d1f8d449e683f34696c6cd4/src/themeDescriptions/base/vk.ts#L730-L741

Вопросы

1.

Стоит ли нам заменять на токены отступы в режиме removable такие как 44px и 48px?

/* размер removable icon */
padding-inline-end: 44px;

padding-inline: 48px var(--vkui--size_base_padding_horizontal--regular);

Так как у нас нет соответствующих токенов для таких размеров, то можно было бы использовать сложение имеющихся токенов, например
вместо 44px писать calc(var(--vkui--spacing_size_3xl) + var(--vkui--spacing_size_4xl))
а вместо 48px писать calc(var(--vkui--spacing_size_4xl) * 2)

Ответ: в данном случае оставим как есть, потому что тут не расстояние а размер иконки, и не хотелось бы, что это поехало при изменении токенов расстояний.

2.

У RichCell есть нестандартный отступ у группы bottom - 5px. Оставим как есть или может быть в дизайне до 6px увеличим?

.RichCell__bottom {
margin-block-start: 5px;
}

Ответ: решили использовать --vkui--spacing_size_s (6px) для того чтобы всё привести к единой системе.

Copy link
Contributor

github-actions bot commented Jun 19, 2024

size-limit report 📦

Path Size
JS 368.61 KB (0%)
JS (gzip) 112.85 KB (0%)
JS (brotli) 92.85 KB (0%)
JS import Div (tree shaking) 1.42 KB (0%)
CSS 290.38 KB (+0.2% 🔺)
CSS (gzip) 37.74 KB (+0.23% 🔺)
CSS (brotli) 30.49 KB (+0.15% 🔺)

Copy link

codesandbox-ci bot commented Jun 19, 2024

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.

Copy link
Contributor

github-actions bot commented Jun 19, 2024

e2e tests

⚠️ Some screenshots were failed. See Playwright Report.

Playwright Report

Copy link
Contributor

github-actions bot commented Jun 19, 2024

👀 Docs deployed

Commit caf76a8

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.78%. Comparing base (9ba0c02) to head (caf76a8).

Current head caf76a8 differs from pull request most recent head b993feb

Please upload reports for the commit b993feb to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7033   +/-   ##
=======================================
  Coverage   83.78%   83.78%           
=======================================
  Files         357      357           
  Lines       10676    10676           
  Branches     3551     3551           
=======================================
  Hits         8945     8945           
  Misses       1731     1731           
Flag Coverage Δ
unittests 83.78% <ø> (ø)

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.

@mendrew mendrew changed the title feat(Cell/SimpleCell/MiniInfoCell) Use spacing size tokens feat(Cell/SimpleCell/MiniInfoCell): Use spacing size tokens Jun 20, 2024
@mendrew mendrew added this to the v6.2.0 milestone Jun 20, 2024
@mendrew mendrew self-assigned this Jun 20, 2024
@mendrew mendrew changed the title feat(Cell/SimpleCell/MiniInfoCell): Use spacing size tokens feat(Cell/SimpleCell/MiniInfoCell/RichCell): Use spacing size tokens Jun 20, 2024
@mendrew mendrew marked this pull request as ready for review June 20, 2024 10:10
@mendrew mendrew requested a review from a team as a code owner June 20, 2024 10:10
@mendrew mendrew requested a review from a team June 20, 2024 10:10
inomdzhon
inomdzhon previously approved these changes Jun 20, 2024
Copy link
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

👍


Ответы на вопросы:

  1. Вообще можно было бы комбинировать, но боюсь, что и читать сложнее станет, и конечному пользователю будет переопределять сложнее. Предлагаю пока оставить как есть.
  2. @VKCOM/vkui-design ?

@mendrew mendrew force-pushed the mendrew/spacing-token/Cell/MiniInfoCell/SimpleCell branch from 0a4afbd to caf76a8 Compare June 28, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants