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(Cell): add auto scroll for draggable mode #5833

Merged

Conversation

inomdzhon
Copy link
Contributor

@inomdzhon inomdzhon commented Sep 19, 2023

Описание

  • Для оптимизации:
    • многие переменные и события инициализирую при onDragMove и очищаю при onDragEnd;
    • перетаскиваемому элементу задаю position: fixed (как в библиотеке https://github.com/hello-pangea/dnd). Если задавать position: absolute или оставить только translateY, то при перетаскивании элемента к нижнему краю, увеличивается scrollHeight.
  • При скролле смещение прибавляем разницу scrollTop до и после к смещению clientY, который мы запоминаем в lastClientYRef.
  • Автоскролл начинается через 300мс при условии, что мы находимся в 50px от краёв родителя со скроллом. Скорость автоскролла зависит от того, насколько близко бы находимся к краям (решение ).
  • Помимо автоскролла, при перетаскивании элемента к границам области видимости по вертикали, добавил возможность скроллить с помощью колёсика мыши.

Кейсы

Кейс 1

Элементом со скроллом является window.

Пример на dekstop

global-scroll-desktop.mov

Пример в mobile

global-scroll-mobile.mov

Кейс 2

Элементом со скроллом является НЕ window.

Пример на dekstop

local-scroll-desktop.mov

Пример в mobile

local-scroll-mobile.mov

Ограничения

  • Не учитывается кейс, когда у нас несколько родителей со скроллом.

Изменения

  • Удалил ListContext, т.к. перенёс установку transition и pointer-events в JS код.
  • Использую утилитарные функции из @vkontakte/vkui-floating-ui.
  • Перенёс хук useDraggable и его зависимости в папаку hooks/ и переименовал в useDraggableDomApi, т.к., чтобы не делать брейкинг чендж, на текущий момент проще было сделать через DOM API, вместо React Way.
  • Jest
    • В packages/vkui/jest.setup.js полифилю DOMRect. Также туда перенёс mathMedia.
    • В packages/vkui/src/testing/utils.ts создал помощники для requestAnimationFrame (waitRAF и requestAnimationFrameMock).

@inomdzhon inomdzhon added ci:cherry-pick:patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча v5 Автоматизация: PR продублируется в ветку v5 labels Sep 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2023

size-limit report 📦

Path Size
JS 341.2 KB (+1.43% 🔺)
JS (gzip) 104.19 KB (+1.41% 🔺)
JS (brotli) 86.08 KB (+1.57% 🔺)
JS import Div (tree shaking) 2.71 KB (0%)
CSS 255.2 KB (-0.02% 🔽)
CSS (gzip) 33.54 KB (-0.1% 🔽)
CSS (brotli) 27.24 KB (-0.06% 🔽)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 19, 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 0e30d8f:

Sandbox Source
VKUI TypeScript Configuration

@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2023

e2e tests

Playwright Report

@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2023

👀 Docs deployed

Commit 0e30d8f

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Attention: 123 lines in your changes are missing coverage. Please review.

Comparison is base (107c29d) 79.38% compared to head (0e30d8f) 79.00%.

Files Patch % Lines
...s/useDraggableWithDomApi/useDraggableWithDomApi.ts 32.14% 114 Missing ⚠️
packages/vkui/src/lib/dom.tsx 84.00% 8 Missing ⚠️
packages/vkui/src/lib/rafSchd.ts 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5833      +/-   ##
==========================================
- Coverage   79.38%   79.00%   -0.39%     
==========================================
  Files         306      311       +5     
  Lines        9631     9903     +272     
  Branches     3261     3314      +53     
==========================================
+ Hits         7646     7824     +178     
- Misses       1985     2079      +94     
Flag Coverage Δ
unittests 79.00% <67.02%> (-0.39%) ⬇️

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.

@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5368/fix/useDraggable-auto-scroll branch 4 times, most recently from e8c6fda to 7efd20d Compare September 22, 2023 13:17
@github-actions github-actions bot added the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Oct 3, 2023
@inomdzhon inomdzhon added no-stale Добавляет PR в исключения для автоматического закрытия and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Oct 9, 2023
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5368/fix/useDraggable-auto-scroll branch from 7efd20d to b6ffed0 Compare October 12, 2023 08:17
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5368/fix/useDraggable-auto-scroll branch from b6ffed0 to 9c6d6de Compare October 23, 2023 13:07
@inomdzhon inomdzhon marked this pull request as ready for review October 23, 2023 13:14
@inomdzhon inomdzhon requested a review from a team as a code owner October 23, 2023 13:14
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.

Потрясающая работа! 👏 👏 👏

Работает прекрасно! Очень непростая задача, как по мне!
Сделано классно! 🔥🔥🔥 Спасибо за тесты 💯

Единственное, что стоит внимательнее посмотреть, это факт того, что иногда тригерится несколько recalculation style в коде, отвечающем за сдвиг элементов при драге. От того, что для определения необходимости сдвига мы читаем размеры и позицию элементов, потом меняем стили, а затем снова читаем размеры того же или уже других элементов и снова устанавливаем стили.
В идеале мы должны сначала все значения прочитать, а потом уже сразу для всех элементов поменять стили, иначе браузеру, для того, чтобы в очередной раз прочитать размеры элемента после изменения стилей приходится пересчитывать геометрию, чтоб понять затронули ли последние изменения элемент и предоставить актуальные размеры.

Возможно, что это nitpick и не стоит тратить время, потому что результаты и так хорошие, в большинстве вызовом dramove recalculation style вызывается один раз, но мне сложно представить как это будет в более сложном реальном примере.

@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5368/fix/useDraggable-auto-scroll branch from 9c6d6de to beb3d9b Compare November 3, 2023 16:56
Copy link
Contributor

@SevereCloud SevereCloud 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 marked this pull request as draft November 15, 2023 09:28
- Импортиурем файл с утилитами
- Добавляем запуск для конткретных движков
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5368/fix/useDraggable-auto-scroll branch from b8645c4 to 1744156 Compare November 15, 2023 14:30
@inomdzhon inomdzhon force-pushed the imirdzhamolov/issue5368/fix/useDraggable-auto-scroll branch from 1744156 to 9d0aacb Compare November 15, 2023 14:51
@inomdzhon inomdzhon marked this pull request as ready for review November 15, 2023 23:19
@SevereCloud SevereCloud self-requested a review November 16, 2023 08:56
SevereCloud
SevereCloud previously approved these changes Nov 16, 2023
Copy link
Contributor

@SevereCloud SevereCloud 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 removed ci:cherry-pick:patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча no-stale Добавляет PR в исключения для автоматического закрытия labels Nov 16, 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.

Потрясающая работа 👏 👏 👏

Спасибо!

@inomdzhon inomdzhon merged commit 43e5c5c into master Nov 20, 2023
@inomdzhon inomdzhon deleted the imirdzhamolov/issue5368/fix/useDraggable-auto-scroll branch November 20, 2023 08:16
vkcom-publisher pushed a commit that referenced this pull request Nov 20, 2023
* tech: extend e2e test infra

- Импортиурем файл с утилитами
- Добавляем запуск для конткретных движков

* feat(Cell): add auto scroll for draggable mode

* fix(review): reword checkIfElementIsInsideYEdgesOfViewport args

* fix(review): refactor useDraggable

* chore: add tests

* chore: add comments with references

* refactor: revert now() moving to lib/date

* review: mv condition for icon to var

* review: extend dom.test.ts
inomdzhon added a commit that referenced this pull request May 28, 2024
- caused by #5833

Удаляем проверку на OUTBOX_OFFSET, т.к. из-за неё нельзя включить автоскролл при уводе элемента за область видимости.
@inomdzhon inomdzhon mentioned this pull request May 28, 2024
1 task
inomdzhon added a commit that referenced this pull request May 28, 2024
- caused by #5833

Удаляем проверку на OUTBOX_OFFSET, т.к. из-за неё нельзя включить автоскролл при уводе элемента за область видимости.
vkcom-publisher pushed a commit that referenced this pull request May 28, 2024
- caused by #5833

Удаляем проверку на OUTBOX_OFFSET, т.к. из-за неё нельзя включить автоскролл при уводе элемента за область видимости.
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
Development

Successfully merging this pull request may close these issues.

[Bug]: Не реализован автоскролл компонента Cell
3 participants