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(View): increase swipe back zone #5725

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

inomdzhon
Copy link
Contributor

@inomdzhon inomdzhon commented Sep 1, 2023

Увеличил зону срабатывания свайпа назад, тем самым приблизив поведение к нативным приложениям. В частности, как в приложении ВКонтакте на iOS.

Note

Для ViewInfitnite повторил всё, что для View.

Пример

view-example.mov

Побочные изменения

Спасибо @shevchux и @mendrew за замечания ❤️ Иначе пропустил бы эти кейсы)

Во-первых, немного пошатал тесты.

Во-вторых, оптимизировал код: убрал из зависимостей useEffect лишнее; состояние swipeBackPrevented теперь храню в useRef вместо useState.

Во-третьих, поправил следующие кейсы:

Кейс 1. Элементы с горизонтальным скроллом.

Добавил обнаружение смещения горизонтального скролла (в частности необходимо для HorizontallScroll), чтобы предотвращать свайп бек если пользователей взаимодействует с элементами, у которых есть горизонтальный скролл.

Свайп бек включается:

  • либо если положение скролла находится в начале, иначе свайп бэк игнорируется. Если мы проскролили к началу, то нам необходимо отжать палец и заново синицировать свайп бэк, чтобы он сработал;
  • либо если потянули за край экрана слева (спасибо, @shevchux за предложение).

Вдохновился поведением у приложения ВКонтакте на iOS.

Использую состояние swipeBackPrevented, чтобы вызывать функцию обнаружения только один раз во время начала жеста. Чтобы получить родителей со скроллом, применяю утилиту getOverflowAncestors() из бибилотеки Floating UI.

Пример

view-horizontall-scroll-example.mov

Кейс 2. Компонент Gallery.

Добавил stopPropagation() на onDragStart для Gallery, чтобы предотвращать свайп бэк.

Пример

view-gallery-example.mov

UPD

Добавил в доки View (стайлгайд и сторибук) примеры с кейсами 1 и 2.

По замечанию @shevchux, в доке у CellButton'ов выставил stopPropagation={false}, чтобы не путало пользователей. Иначе свайп бэк блокируется.

image

UPD

@inomdzhon inomdzhon requested a review from a team as a code owner September 1, 2023 12:06
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 1, 2023

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.

Latest deployment of this branch, based on commit 03ad12c:

Sandbox Source
VKUI TypeScript Configuration

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

size-limit report 📦

Path Size
JS 346.9 KB (+0.24% 🔺)
JS (gzip) 103.58 KB (+0.35% 🔺)
JS (brotli) 84.4 KB (+0.09% 🔺)
JS import Div (tree shaking) 3.04 KB (0%)
CSS 278.5 KB (0%)
CSS (gzip) 36.33 KB (0%)
CSS (brotli) 28.74 KB (0%)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

e2e tests

Playwright Report

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

👀 Docs deployed

Commit 03ad12c

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage: 84.00% and project coverage change: +0.21% 🎉

Comparison is base (a3da40d) 81.99% compared to head (03ad12c) 82.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5725      +/-   ##
==========================================
+ Coverage   81.99%   82.20%   +0.21%     
==========================================
  Files         298      298              
  Lines        9839     9865      +26     
  Branches     3115     3118       +3     
==========================================
+ Hits         8067     8110      +43     
+ Misses       1772     1755      -17     
Flag Coverage Δ
unittests 82.20% <84.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
packages/vkui/src/components/View/View.tsx 90.39% <81.25%> (-2.63%) ⬇️
packages/vkui/src/components/View/ViewInfinite.tsx 87.18% <82.05%> (-4.15%) ⬇️
...es/vkui/src/components/BaseGallery/BaseGallery.tsx 74.69% <100.00%> (+16.92%) ⬆️
packages/vkui/src/components/View/utils.ts 100.00% <100.00%> (ø)
packages/vkui/src/lib/floating/index.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mendrew
Copy link
Contributor

mendrew commented Sep 1, 2023

Круто!

@inomdzhon А с горизонтальными скроллбарами проверил?

Корнер: горизонтальные скроллбары с ненулевым scrollX не должны конфликтовать со свайпбеком. Наверное, можно оставить на ответственности разработчика, но стоит упомянуть в документации. Где-нибудь рядом с data-vkui-swipe-back={false}

@inomdzhon inomdzhon marked this pull request as draft September 1, 2023 13:21
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5583/feat/View-swipe-area branch 3 times, most recently from 1562253 to 78d920a Compare September 1, 2023 18:26
@inomdzhon inomdzhon marked this pull request as ready for review September 1, 2023 18:34
@inomdzhon
Copy link
Contributor Author

Круто!

@inomdzhon А с горизонтальными скроллбарами проверил?

Корнер: горизонтальные скроллбары с ненулевым scrollX не должны конфликтовать со свайпбеком. Наверное, можно оставить на ответственности разработчика, но стоит упомянуть в документации. Где-нибудь рядом с data-vkui-swipe-back={false}

Ой .... только сейчас заметил коммент 😰 Угу, как раз перевёл в драфт, чтобы поправить горизотныльный скролл и Gallery)

Спасибо за замечание 🙏

@inomdzhon inomdzhon marked this pull request as draft September 4, 2023 09:04
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5583/feat/View-swipe-area branch 2 times, most recently from 31c78f6 to d81d5a1 Compare September 5, 2023 14:53
@SevereCloud SevereCloud added the v5 Автоматизация: PR продублируется в ветку v5 label Sep 6, 2023
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5583/feat/View-swipe-area branch from d81d5a1 to b38f3b2 Compare September 7, 2023 12:03
@inomdzhon inomdzhon marked this pull request as ready for review September 7, 2023 13:41
mendrew
mendrew previously approved these changes Sep 12, 2023
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.

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

Тесты хороши 👍

packages/vkui/src/components/View/utils.ts Show resolved Hide resolved
packages/vkui/src/components/View/View.test.tsx Outdated Show resolved Hide resolved
packages/vkui/src/components/View/ViewInfinite.test.tsx Outdated Show resolved Hide resolved
Увеличил зону срабатывания свайпа назад, тем самым приблизив
поведение к нативным приложениям. В частности, как в
приложении ВКонтакте на iOS.
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5583/feat/View-swipe-area branch from b38f3b2 to 03ad12c Compare September 12, 2023 11:08
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 9525ded into master Sep 19, 2023
24 checks passed
@inomdzhon inomdzhon deleted the imirdzhamolov/issue5583/feat/View-swipe-area branch September 19, 2023 16:15
vkcom-publisher pushed a commit that referenced this pull request Sep 19, 2023
Увеличил зону срабатывания свайпа назад, тем самым приблизив поведение к нативным приложениям. В частности, как в приложении ВКонтакте на iOS.

> **Note**
>
> Для `ViewInfitnite` повторил всё, что для `View`.

h.2 Побочные изменения

> Спасибо @shevchux и @mendrew за замечания ❤️ Иначе пропустил бы эти кейсы)

Во-первых, немного пошатал тесты.

Во-вторых, оптимизировал код: убрал из зависимостей `useEffect` лишнее; состояние `swipeBackPrevented` теперь храню в `useRef` вместо `useState`.

Во-третьих, поправил следующие кейсы:

h.3 Кейс 1. Элементы с горизонтальным скроллом.

Добавил обнаружение смещения горизонтального скролла (в частности необходимо для `HorizontallScroll`), чтобы предотвращать свайп бек если пользователей взаимодействует с элементами, у которых есть горизонтальный скролл.

Свайп бек включается:
- либо если положение скролла находится в начале, иначе свайп бэк игнорируется. Если мы проскролили к началу, то нам необходимо отжать палец и заново синицировать свайп бэк, чтобы он сработал;
- либо если потянули за край экрана слева (спасибо, @shevchux за предложение).

> Вдохновился поведением у приложения ВКонтакте на iOS.

> Использую состояние `swipeBackPrevented`, чтобы вызывать функцию обнаружения только один раз во время начала жеста. Чтобы получить родителей со скроллом, применяю утилиту [getOverflowAncestors()](https://github.com/floating-ui/floating-ui/blob/%40floating-ui/react-dom%402.0.2/packages/utils/test/getOverflowAncestors.test.ts) из бибилотеки Floating UI.

h.3 Кейс 2. Компонент [Gallery](https://vkcom.github.io/VKUI/5.7.2/#/Gallery).

Добавил `stopPropagation()` на `onDragStart` для `Gallery`, чтобы предотвращать свайп бэк.
mendrew added a commit that referenced this pull request Dec 26, 2023
Мы очень хитро восстанавливаем позицию скролла, надеясь на то, что это всегда будет работать в useEffect, который сработает перед обработкой события transition (перед завершением анимации), где окончательно сбросятся все состояния.
Но в случае, если пользователь вернул панель назад, мы просто сбрасываем состояние компонента, отвечающее, за свайп. Логика, отвечающая за восстановление скролла не срабатывает, потому что это не считается как `failure`.

Вот тут мы решаем `swipeBack` был успешным (`success`), был отменён (`fail`), то есть свайп не закончен, либо это вообще не считается свайпом, потому что панель по завершении жеста осталась на той же позиции.
https://github.com/VKCOM/VKUI/blob/a4719b49f887c2584eec6655d72e373e62409c59/packages/vkui/src/components/View/View.tsx#L301-L316
Если это не свайп вовсе (пользователь вернул панель на место), то мы просто сбрасываем состояние свайпа с помощью функции `onSwipeBackCancel`.

Но в такой ситуации не сработает условие для восстановления скролла при отмене свайпа.
https://github.com/VKCOM/VKUI/blob/8dbb1de9855af8c772abcb719848175654e39a8a/src/components/View/View.tsx#L494-L500

- caused by #5725

-- Изменения
Вынес логику по восстановлению скролла при отмене свайпа в отдельный useEffect, потому что изначальный слишком большой.
Смотрю на переменные `prevSwipingBack`, `swipingBack`, чтобы понять был ли всё же свайп, потому что `swipeBackResult` нам ни о чем не скажет, даже если бы мы его устанавливали, то он не был бы тут же очищен в том же рендере из-за вызова `onSwipeBackCancel`. Также проверяю `prevSwipeBackShift`, который равен нулю, то есть смещения после жеста у панели нет, что значит, что пользователь жестом свайпа вернул панель туда откуда взял.
mendrew added a commit that referenced this pull request Jan 17, 2024
Cherry-picked из v6.

Мы очень хитро восстанавливаем позицию скролла, надеясь на то, что это всегда будет работать в useEffect, который сработает перед обработкой события transition (перед завершением анимации), где окончательно сбросятся все состояния.
Но в случае, если пользователь вернул панель назад, мы просто сбрасываем состояние компонента, отвечающее, за свайп. Логика, отвечающая за восстановление скролла не срабатывает, потому что это не считается как `failure`.

Вот тут мы решаем `swipeBack` был успешным (`success`), был отменён (`fail`), то есть свайп не закончен, либо это вообще не считается свайпом, потому что панель по завершении жеста осталась на той же позиции.
https://github.com/VKCOM/VKUI/blob/a4719b49f887c2584eec6655d72e373e62409c59/packages/vkui/src/components/View/View.tsx#L301-L316
Если это не свайп вовсе (пользователь вернул панель на место), то мы просто сбрасываем состояние свайпа с помощью функции `onSwipeBackCancel`.

Но в такой ситуации не сработает условие для восстановления скролла при отмене свайпа.
https://github.com/VKCOM/VKUI/blob/8dbb1de9855af8c772abcb719848175654e39a8a/src/components/View/View.tsx#L494-L500

- caused by #5725

-- Изменения
Вынес логику по восстановлению скролла при отмене свайпа в отдельный useEffect, потому что изначальный слишком большой.
Смотрю на переменные `prevSwipingBack`, `swipingBack`, чтобы понять был ли всё же свайп, потому что `swipeBackResult` нам ни о чем не скажет, даже если бы мы его устанавливали, то он не был бы тут же очищен в том же рендере из-за вызова `onSwipeBackCancel`. Также проверяю `prevSwipeBackShift`, который равен нулю, то есть смещения после жеста у панели нет, что значит, что пользователь жестом свайпа вернул панель туда откуда взял.
mendrew added a commit that referenced this pull request Jan 18, 2024
Cherry-picked из v6.

Мы очень хитро восстанавливаем позицию скролла, надеясь на то, что это всегда будет работать в useEffect, который сработает перед обработкой события transition (перед завершением анимации), где окончательно сбросятся все состояния.
Но в случае, если пользователь вернул панель назад, мы просто сбрасываем состояние компонента, отвечающее, за свайп. Логика, отвечающая за восстановление скролла не срабатывает, потому что это не считается как `failure`.

Вот тут мы решаем `swipeBack` был успешным (`success`), был отменён (`fail`), то есть свайп не закончен, либо это вообще не считается свайпом, потому что панель по завершении жеста осталась на той же позиции.
https://github.com/VKCOM/VKUI/blob/a4719b49f887c2584eec6655d72e373e62409c59/packages/vkui/src/components/View/View.tsx#L301-L316
Если это не свайп вовсе (пользователь вернул панель на место), то мы просто сбрасываем состояние свайпа с помощью функции `onSwipeBackCancel`.

Но в такой ситуации не сработает условие для восстановления скролла при отмене свайпа.
https://github.com/VKCOM/VKUI/blob/8dbb1de9855af8c772abcb719848175654e39a8a/src/components/View/View.tsx#L494-L500

- caused by #5725

-- Изменения
Вынес логику по восстановлению скролла при отмене свайпа в отдельный useEffect, потому что изначальный слишком большой.
Смотрю на переменные `prevSwipingBack`, `swipingBack`, чтобы понять был ли всё же свайп, потому что `swipeBackResult` нам ни о чем не скажет, даже если бы мы его устанавливали, то он не был бы тут же очищен в том же рендере из-за вызова `onSwipeBackCancel`. Также проверяю `prevSwipeBackShift`, который равен нулю, то есть смещения после жеста у панели нет, что значит, что пользователь жестом свайпа вернул панель туда откуда взял.
vkcom-publisher pushed a commit that referenced this pull request Jan 18, 2024
Cherry-picked из v6.

Мы очень хитро восстанавливаем позицию скролла, надеясь на то, что это всегда будет работать в useEffect, который сработает перед обработкой события transition (перед завершением анимации), где окончательно сбросятся все состояния.
Но в случае, если пользователь вернул панель назад, мы просто сбрасываем состояние компонента, отвечающее, за свайп. Логика, отвечающая за восстановление скролла не срабатывает, потому что это не считается как `failure`.

Вот тут мы решаем `swipeBack` был успешным (`success`), был отменён (`fail`), то есть свайп не закончен, либо это вообще не считается свайпом, потому что панель по завершении жеста осталась на той же позиции.
https://github.com/VKCOM/VKUI/blob/a4719b49f887c2584eec6655d72e373e62409c59/packages/vkui/src/components/View/View.tsx#L301-L316
Если это не свайп вовсе (пользователь вернул панель на место), то мы просто сбрасываем состояние свайпа с помощью функции `onSwipeBackCancel`.

Но в такой ситуации не сработает условие для восстановления скролла при отмене свайпа.
https://github.com/VKCOM/VKUI/blob/8dbb1de9855af8c772abcb719848175654e39a8a/src/components/View/View.tsx#L494-L500

- caused by #5725

-- Изменения
Вынес логику по восстановлению скролла при отмене свайпа в отдельный useEffect, потому что изначальный слишком большой.
Смотрю на переменные `prevSwipingBack`, `swipingBack`, чтобы понять был ли всё же свайп, потому что `swipeBackResult` нам ни о чем не скажет, даже если бы мы его устанавливали, то он не был бы тут же очищен в том же рендере из-за вызова `onSwipeBackCancel`. Также проверяю `prevSwipeBackShift`, который равен нулю, то есть смещения после жеста у панели нет, что значит, что пользователь жестом свайпа вернул панель туда откуда взял.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v5 Автоматизация: PR продублируется в ветку v5
Projects
None yet
3 participants