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(PanelHeaderButton): refactor pressets #7874

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

Conversation

EldarMuhamethanov
Copy link
Contributor

@EldarMuhamethanov EldarMuhamethanov commented Oct 31, 2024

  • Unit-тесты
  • e2e-тесты
  • Документация фичи
  • Release notes
  • Codemods

Описание

В настоящее время пресеты компонента PanelHeaderButton демонстрируют несогласованное поведение при использовании пропсов label и children на разных платформах.

Изменения

Унифицировано поведение пресетов для обеспечения предсказуемого отображения на всех платформах.

Основные изменения

Поведение label:

  • iOS: текст отображается
  • Android/Desktop: текст визуально скрыт, но доступен для скринридеров

Удаление children:

  • Удален проп children у всех пресетов
  • Иконки теперь встроены в пресеты
  • Текстовый контент передается только через label

Особый случай: PanelHeaderBack

  • Сохранено специальное поведение для отображения label на VKCOM
  • Добавлены новые пропсы для гибкой настройки в разных кейсах:
    • hideLabelOnVKCom - скрывает визуально label на платформе vkcom
    • hideLabelOnIOS - скрывает визуально label на платформе iOS

Документация

  • Обновил примеры в документации и текст описывающие пресеты

Дополнительные изменения

  • Обновлена документация с описанием поведения на разных платформах
  • Написаны кодмоды для автоматической миграции
  • Обновлены скриншоты тестов

UPD

Пришлось оставить исключение PanelHeaderButton, так как у него есть особенная логика - label иногда должен отображаться не только на IOS, но и на VKCOM. Также было бы неплохо, чтобы можно было управлять тем, нужно ли скрывать label на разных платформах, поэтому добавил свойства hideLabelOnVKCom и hideLabelOnIOS для более точно найстройки в зависимости от кейса.

Release notes

BREAKING CHANGE

  • PanelHeaderButton:
    • у пресета PanelHeaderClose удалено свойство children. Теперь для прокидывания текста для a11y нужно прокидывать его в свойствоlabel
    • у пресета PanelHeaderSubmit удалено свойство children. Теперь для прокидывания текста для a11y нужно прокидывать его в свойствоlabel
    • у пресета PanelHeaderEdit удалены свойства children и label. Вместо label можно использовать свойства doneLabel и editLabel. Свойство children не использовалось.
    • у пресета PanelHeaderBack удалено свойство children. Теперь для прокидывания текста для a11y нужно прокидывать его в свойство label. Логика отображения label осталась как была, в отличае от других пресетов. Также для более точно настройки label были добавлены свойства hideLabelOnVKCom и hideLabelOnIOS, чтобы можно было скрывать label на соответствующей платформе.

Copy link
Contributor

github-actions bot commented Oct 31, 2024

size-limit report 📦

Path Size
JS 385.56 KB (+0.02% 🔺)
JS (gzip) 123.26 KB (+0.06% 🔺)
JS (brotli) 102.15 KB (+0.03% 🔺)
JS import Div (tree shaking) 1.47 KB (0%)
CSS 342.52 KB (0%)
CSS (gzip) 49.37 KB (0%)
CSS (brotli) 40.04 KB (0%)

Copy link

codesandbox-ci bot commented Oct 31, 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 Oct 31, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Oct 31, 2024

👀 Docs deployed

Commit fc45f37

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.30%. Comparing base (50960ea) to head (fc45f37).
Report is 40 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7874      +/-   ##
==========================================
+ Coverage   95.16%   95.30%   +0.14%     
==========================================
  Files         376      376              
  Lines       11008    10979      -29     
  Branches     3653     3669      +16     
==========================================
- Hits        10476    10464      -12     
+ Misses        532      515      -17     
Flag Coverage Δ
unittests 95.30% <100.00%> (+0.14%) ⬆️

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.

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.

👏 💅

packages/vkui/src/components/PanelHeaderButton/Readme.md Outdated Show resolved Hide resolved
const platform = usePlatform();
const { sizeX = 'none' } = useAdaptivity();
// также label нужно скрывать при platform === 'ios' && sizeX === regular
// https://github.com/VKCOM/VKUI/blob/master/src/components/PanelHeaderButton/PanelHeaderButton.css#L104
const showLabel = platform === 'vkcom' || platform === 'ios';
const showLabel = platform === 'ios';
Copy link
Contributor

Choose a reason for hiding this comment

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

В #1208 добавляли platform === "vkcom" считая отсутствие лейбла на VKCOM багом

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Хм, уточню у дизайна про этот компонент. Судя по всему ему нужна своя кастомная логика, так как судя по скришотам измененным выглядит не очень

@inomdzhon inomdzhon self-requested a review November 5, 2024 09:29
return (
(<React.Fragment>
<PanelHeaderBack onClick={noop} label={"Закрыть"}></PanelHeaderBack>
<PanelHeaderBack onClick={noop} label={<><span>Закрыть</span></>}></PanelHeaderBack>
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.

в целом да, там просто проблема с переносом из children в label. По-другому сделать довольно проблематично. И возможно это линтер исправить сможет

@EldarMuhamethanov EldarMuhamethanov marked this pull request as draft November 6, 2024 07:10
@EldarMuhamethanov EldarMuhamethanov requested a review from a team November 13, 2024 09:40
@EldarMuhamethanov EldarMuhamethanov marked this pull request as ready for review November 13, 2024 09:41
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.

3 participants