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(Popover): add keepMount props #7058

Merged
merged 14 commits into from
Jun 26, 2024

Conversation

EldarMuhamethanov
Copy link
Contributor


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

Описание

Добавил пропс keepMount, при выставлении которого, поповер не будет удаляться из DOM-дерева, а будет скрываться только визуально.
Добавил тест для компонента Popover, в котором проверил, новый функционал

Добавил пропс с помощью которого поповер не удаляется из DOM дерева при скрытии
@EldarMuhamethanov EldarMuhamethanov requested a review from a team as a code owner June 24, 2024 07:22
Copy link
Contributor

github-actions bot commented Jun 24, 2024

size-limit report 📦

Path Size
JS 364.74 KB (+0.09% 🔺)
JS (gzip) 111.71 KB (+0.14% 🔺)
JS (brotli) 91.92 KB (+0.08% 🔺)
JS import Div (tree shaking) 1.42 KB (0%)
CSS 285.3 KB (+0.02% 🔺)
CSS (gzip) 37 KB (+0.02% 🔺)
CSS (brotli) 29.94 KB (+0.09% 🔺)

Copy link

codesandbox-ci bot commented Jun 24, 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 24, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Jun 24, 2024

👀 Docs deployed

Commit 928527c

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.61%. Comparing base (4ee1f30) to head (928527c).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7058      +/-   ##
==========================================
+ Coverage   83.57%   83.61%   +0.03%     
==========================================
  Files         352      352              
  Lines       10547    10569      +22     
  Branches     3489     3497       +8     
==========================================
+ Hits         8815     8837      +22     
  Misses       1732     1732              
Flag Coverage Δ
unittests 83.61% <100.00%> (+0.03%) ⬆️

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.

Co-authored-by: Daniil Suvorov <severecloud@gmail.com>
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.

Гляжу FocusTrap глючит если неаманутить его...

Через мышку:

  1. передаём keepMounted;
  2. вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. вызываем Popover – он показывается, но нет фокуса на него;

Через клавиатуру:

  1. передаём keepMounted;
  2. через Tab + Click вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. снова через Tab + Click пытаемся сфокусироваться Popover – но уже не получается.

packages/vkui/src/components/Popover/Popover.tsx Outdated Show resolved Hide resolved
@EldarMuhamethanov
Copy link
Contributor Author

Гляжу FocusTrap глючит если неаманутить его...

Через мышку:

  1. передаём keepMounted;
  2. вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. вызываем Popover – он показывается, но нет фокуса на него;

Через клавиатуру:

  1. передаём keepMounted;
  2. через Tab + Click вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. снова через Tab + Click пытаемся сфокусироваться Popover – но уже не получается.

Поправил данную проблему путем добавления пропса disableTrap, для того, чтобы вырубать эффект захвата фокуса когда поповер визуально скрыт, а также чтобы при появлении поповера происходил автофокус

@inomdzhon
Copy link
Contributor

Гляжу FocusTrap глючит если неаманутить его...
Через мышку:

  1. передаём keepMounted;
  2. вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. вызываем Popover – он показывается, но нет фокуса на него;

Через клавиатуру:

  1. передаём keepMounted;
  2. через Tab + Click вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. снова через Tab + Click пытаемся сфокусироваться Popover – но уже не получается.

Поправил данную проблему путем добавления пропса disableTrap, для того, чтобы вырубать эффект захвата фокуса когда поповер визуально скрыт, а также чтобы при появлении поповера происходил автофокус

🔥🔥🔥

Токо надо поправить момент с возвращением фокуса на целевой элемент – сейчас начал теряться

До

2024-06-24.22.16.40.mov

После

2024-06-24.22.17.04.mov


Ещё можешь master себе зарабейзить – там правка по FocusTrap

@EldarMuhamethanov
Copy link
Contributor Author

Гляжу FocusTrap глючит если неаманутить его...
Через мышку:

  1. передаём keepMounted;
  2. вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. вызываем Popover – он показывается, но нет фокуса на него;

Через клавиатуру:

  1. передаём keepMounted;
  2. через Tab + Click вызываем Popover – он показывается и есть фокус на него;
  3. скрываем Popover;
  4. снова через Tab + Click пытаемся сфокусироваться Popover – но уже не получается.

Поправил данную проблему путем добавления пропса disableTrap, для того, чтобы вырубать эффект захвата фокуса когда поповер визуально скрыт, а также чтобы при появлении поповера происходил автофокус

🔥🔥🔥

Токо надо поправить момент с возвращением фокуса на целевой элемент – сейчас начал теряться

До
После
Ещё можешь master себе зарабейзить – там правка по FocusTrap

Поправил восстановление фокуса, когда disabled = true

inomdzhon
inomdzhon previously approved these changes Jun 25, 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.

👏 💪

Note

Если будет желание, то можешь ещё сразу домучить тесты – где надо добавить /* istanbul ignore if: <reason> */, а где-то дописать. Сейчас покрытие 92% у FocusTrap.

https://app.codecov.io/gh/VKCOM/VKUI/pull/7058/blob/packages/vkui/src/components/FocusTrap/FocusTrap.tsx

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.

Очень круто!

Но меня смущает новый проп disabled у FocusTrap. У него уже есть restoreFocus, autoFocus, а теперь ещё и disabled который вернёт фокус если true.

Правда, не получается получше придумать. Есть желание разделить disabled и какой-то режим по умолчанию - restoreFocusOnDisabled.
Хотя выглядит ещё больше как нагромождение. Забудьте.


Я бы тогда пристальнее обратил внимание на то как поведёт себя FocusTrap при изменении disabled с true на false, если превый рендер был с disabled true.
И как поведёт себя если компонент размаунтится с disabled=true.
Чтобы не было ситуации, когда Popovera не видно, потом его совсем размаунтили из DOM и вдруг фокус раз и прыгнул куда-то в старое место, которое FocusTrap когда-то давно запомнил.
Да, и чтобы restoreFocusTo не оказался внезапно устаревшим при возврате фокуса.

packages/vkui/src/components/FocusTrap/FocusTrap.tsx Outdated Show resolved Hide resolved
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.

🔥 🔥 🔥

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.

Отличная работа! 💯 🥇

@inomdzhon inomdzhon merged commit 4dda424 into master Jun 26, 2024
26 checks passed
@inomdzhon inomdzhon deleted the e.muhamethanov/7042/popover-keep-mounted branch June 26, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: добавить в Popover keepMounted опцию
4 participants