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

fix(ModalPage, ModalCard): add prop disableFocusTrap to fix conflict with several FocusTraps #8248

Merged

Conversation

EldarMuhamethanov
Copy link
Contributor

@EldarMuhamethanov EldarMuhamethanov commented Feb 7, 2025


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

Описание

Сейчас, когда поверх одной модалки открывается другая, получается что одновременно начинают работать два FocusTrap, которые начинают конфликтовать и работать некорректно. При нажатии Tab фокус перескакивает с одной модалки на другую. Нужно придумать, как решить эту проблему

Изменения

  • Добавил ModalPage и ModalCard свойство disableFocusTrap , которое позволяет отключать захват фокуса, когда, например, одна модалка перекрывается другой
  • Добавил в доку информацию по использованию disableFocusTrap
  • Добавил тесты для кейса с disableFocusTrap

Release notes

Улучшения

  • ModalPage: Добавлено свойство disableFocusTrap для отключения захвата фокуса
    Нужно для кейса, когда поверх одной модалки открывается другая, чтобы несколько FocusTrap не конфликтовали между собой
  • ModalCard: Добавлено свойство disableFocusTrap для отключения захвата фокуса
    Нужно для кейса, когда поверх одной модалки открывается другая, чтобы несколько FocusTrap не конфликтовали между собой

Исправления

  • Исправлена проблема с конфликтом FocusTrap-ов при нескольких открытых модалках

@EldarMuhamethanov EldarMuhamethanov requested a review from a team as a code owner February 7, 2025 09:03
@github-actions github-actions bot added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Feb 7, 2025
Copy link
Contributor

github-actions bot commented Feb 7, 2025

size-limit report 📦

Path Size
JS 395.19 KB (+0.03% 🔺)
JS (gzip) 119.84 KB (+0.02% 🔺)
JS (brotli) 98.51 KB (-0.03% 🔽)
JS import Div (tree shaking) 1.56 KB (0%)
CSS 347.71 KB (0%)
CSS (gzip) 43.03 KB (0%)
CSS (brotli) 34.37 KB (0%)

Copy link

codesandbox-ci bot commented Feb 7, 2025

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 Feb 7, 2025

e2e tests

Playwright Report

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.54%. Comparing base (164edc2) to head (a079326).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8248   +/-   ##
=======================================
  Coverage   95.53%   95.54%           
=======================================
  Files         404      404           
  Lines       11559    11559           
  Branches     3837     3837           
=======================================
+ Hits        11043    11044    +1     
+ Misses        516      515    -1     
Flag Coverage Δ
unittests 95.54% <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.

Copy link
Contributor

github-actions bot commented Feb 7, 2025

👀 Docs deployed

Commit 9151df4

Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk 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/ModalPage/types.ts Outdated Show resolved Hide resolved
packages/vkui/src/components/ModalCard/types.ts Outdated Show resolved Hide resolved
packages/vkui/src/components/ModalCard/Readme.md Outdated Show resolved Hide resolved
Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk 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 previously approved these changes Feb 11, 2025
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.

🎉 👍

@inomdzhon
Copy link
Contributor

Единственно, теперь у нас две вариации свойств, которые что-то отключают: disableContentPanningGesture и focusTrapDisabled

@EldarMuhamethanov
Copy link
Contributor Author

Единственно, теперь у нас две вариации свойств, которые что-то отключают: disableContentPanningGesture и focusTrapDisabled

Действительно, лучше переименовать, чтобы потом потом не пришлось

@inomdzhon inomdzhon changed the title fix(ModalPage, ModalCard): add prop focusTrapDisabled to fix conflict with several FocusTraps fix(ModalPage, ModalCard): add prop disableFocusTrap to fix conflict with several FocusTraps Feb 11, 2025
Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk left a comment

Choose a reason for hiding this comment

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

💅

@EldarMuhamethanov EldarMuhamethanov merged commit 59c9515 into master Feb 12, 2025
29 checks passed
@EldarMuhamethanov EldarMuhamethanov deleted the e.muhamethanov/8244/fix-focus-traps-conflict branch February 12, 2025 07:56
@vkcom-publisher
Copy link
Contributor

❌ Patch

Не удалось автоматически применить исправление на ветке 7.1-stable.

Дальнейшие действия выполняют контрибьютеры из группы @VKCOM/vkui-core

Чтобы изменение попало в ветку 7.1-stable, выполните следующие действия:

  1. Создайте новую ветку от 7.1-stable и примените изменения используя cherry-pick
git stash # опционально
git fetch origin 7.1-stable
git checkout -b patch/pr8248 origin/7.1-stable

git cherry-pick --no-commit 59c9515ab92faf34a54bdb141f1a0cb18baf755a
git checkout HEAD **/__image_snapshots__/*.png
git diff --quiet HEAD || git commit --no-verify --no-edit
  1. Исправьте конфликты, следуя инструкциям из терминала
  2. Отправьте ветку на GitHub и создайте новый PR с веткой 7.1-stable (установка лейбла не требуется!)
git push --set-upstream origin patch/pr8248
gh pr create --base 7.1-stable --title "patch: pr8248" --body "- patch #8248"

EldarMuhamethanov added a commit that referenced this pull request Feb 12, 2025
…ct with several FocusTraps (#8248)

* fix(ModalPage, ModalCard): add prop `focusTrapDisabled` to fix conflict with several FocusTraps

* doc(ModalPage,ModalCard): fix documentation

* doc(ModalPage,ModalCard): fix docs

* doc(ModalPage,ModalCard): fix docs

* fix: rename `focusTrapDisabled` to `disableFocusTrap`
# Conflicts:
#	packages/vkui/src/components/ModalCard/ModalCard.test.tsx
#	packages/vkui/src/components/ModalCard/ModalCardInternal.tsx
#	packages/vkui/src/components/ModalPage/ModalPage.test.tsx
#	packages/vkui/src/components/ModalPage/types.ts
@EldarMuhamethanov EldarMuhamethanov mentioned this pull request Feb 12, 2025
EldarMuhamethanov added a commit that referenced this pull request Feb 12, 2025
…ct with several FocusTraps (#8248) (#8261)

* fix(ModalPage, ModalCard): add prop `focusTrapDisabled` to fix conflict with several FocusTraps

* doc(ModalPage,ModalCard): fix documentation

* doc(ModalPage,ModalCard): fix docs

* doc(ModalPage,ModalCard): fix docs

* fix: rename `focusTrapDisabled` to `disableFocusTrap`
# Conflicts:
#	packages/vkui/src/components/ModalCard/ModalCard.test.tsx
#	packages/vkui/src/components/ModalCard/ModalCardInternal.tsx
#	packages/vkui/src/components/ModalPage/ModalPage.test.tsx
#	packages/vkui/src/components/ModalPage/types.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча
Projects
None yet
4 participants