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(Link): rm margin from icon #6895

Closed
wants to merge 2 commits into from
Closed

fix(Link): rm margin from icon #6895

wants to merge 2 commits into from

Conversation

inomdzhon
Copy link
Contributor

@inomdzhon inomdzhon commented May 7, 2024 •
edited

Описание

Удаляем margin, т.к.:

  • сейчас расчёт такой, что иконка всегда после текста, а она может быть перед текстом;
  • обычный пробел сам по себе и так компенсирует отступ, чтобы текст и иконка не слипались;
  • по прошедшим e2e тестам видно, что margin не нужен.

Изменения

Заменил в доках обычный пробел на &npsp;, т.к. логичнее использовать его, чтобы иконка не переносилась без текста.

Скриншот

image

@inomdzhon
fix(Link): rm margin from icon
1584e08 
Удаляем `margin`, т.к.:
- сейчас расчёт такой, что иконка всегда после текста, а она может быть перед текстом;
- обычный пробел сам по себе и так компенсирует отступ, чтобы текст и иконка не слипались;
- по прошедшим e2e тестам видно, что `margin` не нужен.
@inomdzhon inomdzhon requested a review from a team as a code owner May 7, 2024 09:33
@github-actions github-actions bot added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label May 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2024 •
edited

size-limit report 📦

Path Size
JS 364.4 KB (0%)
JS (gzip) 111.27 KB (0%)
JS (brotli) 91.79 KB (0%)
JS import Div (tree shaking) 1.43 KB (0%)
CSS 269.79 KB (-0.01% 🔽)
CSS (gzip) 35.28 KB (0%)
CSS (brotli) 28.58 KB (-0.09% 🔽)

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented May 7, 2024 •
edited

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.

mendrew
mendrew previously approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

🔥

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2024 •
edited

👀 Docs deployed

Commit f19dcec

@codecov Codecov
Copy link

codecov bot commented May 7, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.33%. Comparing base (d2b8446) to head (f19dcec).
Report is 9 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #6895   +/-   ##
=======================================
  Coverage   83.33%   83.33%           
=======================================
  Files         350      350           
  Lines       10729    10729           
  Branches     3575     3575           
=======================================
  Hits         8941     8941           
  Misses       1788     1788
Flag Coverage Δ
unittests 83.33% <ø> (ø)

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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2024 •
edited

e2e tests

Playwright Report

@BlackySoul
Copy link
Contributor

А к нам дизайнеры не придут, что теперь иконки сильно липнут к тексту?)
Как будто нужно будет для отступа теперь прокидывать className в иконку, не считается это breaking change? 🤔

@inomdzhon
Copy link
Contributor Author

А к нам дизайнеры не придут, что теперь иконки сильно липнут к тексту?) Как будто нужно будет для отступа теперь прокидывать className в иконку, не считается это breaking change? 🤔

так они не липнут, т.к. есть пробел

e2e тесты бы иначе упали если бы удаление margin-left хоть как-то влияло бы

если нужен отступ больше, то можно добавить два пробела

<Link>Text&nbsp;&nbsp;<Icon /></Link>

@inomdzhon
Copy link
Contributor Author

А к нам дизайнеры не придут, что теперь иконки сильно липнут к тексту?) Как будто нужно будет для отступа теперь прокидывать className в иконку, не считается это breaking change? 🤔

так они не липнут, т.к. есть пробел

e2e тесты бы иначе упали если бы удаление margin-left хоть как-то влияло бы

если нужен отступ больше, то можно добавить два пробела

<Link>Text&nbsp;&nbsp;<Icon /></Link>

@BlackySoul e2e не поехали, т.к. там иконки и не было xDDD поправил)

image

Вот разница

@qurle
Copy link

qurle commented May 13, 2024

А неразрывный пробел будут ставить вручную?

@inomdzhon
Copy link
Contributor Author

inomdzhon commented May 13, 2024 •
edited

А неразрывный пробел будут ставить вручную?

Ага, но зависит от кейса. Если текст небольшой, то обычного пробела хватит. Вообще на вычитке текста копирайтером такие места находятся и правятся.

qurle
qurle approved these changes May 13, 2024
View reviewed changes
Copy link

@qurle qurle left a comment

Choose a reason for hiding this comment

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

Выглядит разумно, если мы подразумеваем иконку как элемент внутри текста. Но потребует вычитки повсюду, это факт. Может, укажем на этот момент в документации?

@inomdzhon
Copy link
Contributor Author

Выглядит разумно, если мы подразумеваем иконку как элемент внутри текста. Но потребует вычитки повсюду, это факт. Может, укажем на этот момент в документации?

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

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

@inomdzhon
Copy link
Contributor Author

@inomdzhon inomdzhon closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qurle qurle qurle approved these changes

@mendrew mendrew mendrew left review comments

Assignees
No one assigned
Labels
patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Применение стилей для иконки внутри <Link>
4 participants
@inomdzhon @BlackySoul @qurle @mendrew