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: add ModalSheet #5092

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

Conversation

SevereCloud
Copy link
Contributor

@SevereCloud SevereCloud commented May 18, 2023

Новый компонент ModalSheet, который использует scroll-snap вместо прошлого способа


Потенциально избавляет от следующих проблем:

@SevereCloud SevereCloud added the no-stale Добавляет PR в исключения для автоматического закрытия label May 18, 2023
@codesandbox-ci
Copy link

codesandbox-ci bot commented May 18, 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.

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Attention: Patch coverage is 78.91892% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 82.14%. Comparing base (eb56a7d) to head (83546b1).
Report is 8 commits behind head on master.

Files Patch % Lines
...ages/vkui/src/components/ModalSheet/ModalSheet.tsx 73.33% 36 Missing ⚠️
packages/vkui/src/hooks/useKeyboard.ts 78.57% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5092      +/-   ##
==========================================
- Coverage   82.20%   82.14%   -0.07%     
==========================================
  Files         331      336       +5     
  Lines       10219    10429     +210     
  Branches     3422     3473      +51     
==========================================
+ Hits         8401     8567     +166     
- Misses       1818     1862      +44     
Flag Coverage Δ
unittests 82.14% <78.91%> (-0.07%) ⬇️

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.

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2023

size-limit report 📦

Path Size
JS 356.35 KB (+1.26% 🔺)
JS (gzip) 108.85 KB (+1.15% 🔺)
JS (brotli) 89.98 KB (+1.07% 🔺)
JS import Div (tree shaking) 1.43 KB (0%)
CSS 262.17 KB (+1.63% 🔺)
CSS (gzip) 34.31 KB (+1.48% 🔺)
CSS (brotli) 27.86 KB (+1.51% 🔺)

@SevereCloud SevereCloud force-pushed the SevereCloud/concept/new-ModalPage branch from d9253b1 to d0d0404 Compare May 18, 2023 09:34
@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2023

e2e tests

Playwright Report

@inomdzhon
Copy link
Contributor

inomdzhon commented May 18, 2023

....неужели... неужели модалки начинают путь к рефактору 🥲 ✨💖✨

Правильно понимаю, что от ModalRoot будем отказываться?


Глянул на iOS, вот что заметил:

  • (коммент не относиться напрямую к iOS) возможно стоит оставлять маску до тех пор, пока модалка полностью не скроется или изменить скорость анимации.
    В конце модалка очень медленно уходит

    Видео 1

    RPReplay_Final1684405594.mp4

  • iOS со своей клавой как всегда добавляет проблем 😭

    Видео 2

    RPReplay_Final1684405641.mp4


Перенёс пример в песочницу https://codesandbox.io/s/pr5092-modalpagenew-438xlz, чтобы было удобней проверять на мобилках.

@SevereCloud
Copy link
Contributor Author

Правильно понимаю, что от ModalRoot будем отказываться?

Хотелось бы, чтобы модальные окна и попапы работали без SplitLayout. Если отказываться от ModalRoot, нужно предложить какую-то систему по открытию нескольких модалок (см #2449)

Перенёс пример в песочницу https://codesandbox.io/s/pr5092-modalpagenew-438xlz, чтобы было удобней проверять на мобилках.

В сторибуке можно открывать в отдельном окне

@inomdzhon
Copy link
Contributor

Правильно понимаю, что от ModalRoot будем отказываться?

Хотелось бы, чтобы модальные окна и попапы работали без SplitLayout. Если отказываться от ModalRoot, нужно предложить какую-то систему по открытию нескольких модалок (см #2449)

Найс

Про систему открытия надо будет покумекать, ну и как в issue написано, нужно ещё с дизайном обсудить

Перенёс пример в песочницу https://codesandbox.io/s/pr5092-modalpagenew-438xlz, чтобы было удобней проверять на мобилках.

В сторибуке можно открывать в отдельном окне

да, просто в codesandbox можно более гибко потыкать, в сторибуке же всё зашито

@SevereCloud SevereCloud force-pushed the SevereCloud/concept/new-ModalPage branch from d0d0404 to 7ab238c Compare August 2, 2023 09:18
@SevereCloud SevereCloud added the v5 Автоматизация: PR продублируется в ветку v5 label Sep 6, 2023
@SevereCloud SevereCloud force-pushed the SevereCloud/concept/new-ModalPage branch from 7ab238c to 596ada9 Compare December 15, 2023 13:15
@SevereCloud SevereCloud removed no-stale Добавляет PR в исключения для автоматического закрытия v5 Автоматизация: PR продублируется в ветку v5 labels Dec 15, 2023
@SevereCloud SevereCloud changed the title concept: new ModalPage feat: add ModalSheet Dec 15, 2023
@SevereCloud SevereCloud marked this pull request as ready for review December 15, 2023 13:16
@SevereCloud SevereCloud requested a review from a team as a code owner December 15, 2023 13:16
Copy link
Contributor

github-actions bot commented Dec 15, 2023

👀 Docs deployed

Commit 83546b1

@levtsypanov
Copy link

levtsypanov commented Dec 15, 2023

Привет! В примере на styleguide с динамической высотой при ширине 320 заметил, что при скроллинге модалка не закрывается, а при раскрытии Accordion-а скроллинг закрывает модалку а не видвигает ее.

@SevereCloud
Copy link
Contributor Author

Привет! В примере на styleguide с динамической высотой при ширине 320 заметил, что при скроллинге модалка не закрывается, а при раскрытии Accordion-а скроллинг закрывает модалку а не видвигает ее.

Фикс

@VKCOM VKCOM deleted a comment from github-actions bot Dec 18, 2023
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.

Выглядит очень клёво! И по реализации выглядит элегантно 💅

Написал несколько предложений.


Проверил на iOS 17.1.2, есть вот такие замечания:

  1. В ModalFirst при раскрытии на весь экран, не выключается scroll-snap (см. Видео).
  2. В ModalDynamic не пересчитывается высота. (см. Видео).
  3. Можно ли сделать так, чтобы при скролле сверху вниз, когда мы проскролили контент, модалка не закрывалась сразу? (см. ModalFull в см. Видео).
Видео

RPReplay_Final1703068228.MP4

packages/vkui/src/components/ModalSheet/Readme.md Outdated Show resolved Hide resolved
packages/vkui/src/hooks/useKeyboard.ts Show resolved Hide resolved
packages/vkui/src/hooks/useKeyboard.ts Outdated Show resolved Hide resolved
packages/vkui/src/components/ModalSheet/ModalSheet.tsx Outdated Show resolved Hide resolved
packages/vkui/src/components/ModalSheet/ModalSheet.tsx Outdated Show resolved Hide resolved
@SevereCloud
Copy link
Contributor Author

  • В ModalFirst при раскрытии на весь экран, не выключается scroll-snap (см. Видео).

Не очень понял, а зачем выключать

  • В ModalDynamic не пересчитывается высота. (см. Видео).

safari...

  • Можно ли сделать так, чтобы при скролле сверху вниз, когда мы проскролили контент, модалка не закрывалась сразу? (см. ModalFull в см. Видео).

Оно уже есть, но оно не реагирует если сильно скролить

@inomdzhon
Copy link
Contributor

  • В ModalFirst при раскрытии на весь экран, не выключается scroll-snap (см. Видео).

Не очень понял, а зачем выключать

Можно на видео наблюдать, что скролл сегментирован, как-будто слайды листаю

Хочется такой же прокрутки как в ModalFull

@SevereCloud
Copy link
Contributor Author

Можно на видео наблюдать, что скролл сегментирован, как-будто слайды листаю

Возможно это кажется из-за обязательной остановки при закрытии модалки. Там просто не может быть снапа

@SevereCloud SevereCloud force-pushed the SevereCloud/concept/new-ModalPage branch from 57be80d to cec5cc7 Compare December 20, 2023 16:49
@BlackySoul
Copy link
Contributor

А это требование какое-то, чтобы при закрытии модалка проскролливалась вверх и только потом закрывалась или баг?)

2023-12-21.20.59.50.mov

@SevereCloud SevereCloud force-pushed the SevereCloud/concept/new-ModalPage branch from 7857084 to 355e9b5 Compare February 26, 2024 09:48
@SevereCloud
Copy link
Contributor Author

закрыть модалку потянув вниз лишь заголовок

Не нашел ничего нативного, что имело бы такое же поведение. Да и реализовывать такое очень не хочется

@shevchux
Copy link

shevchux commented Feb 26, 2024

Анимация открытия и закрытия медленная кажется, не похожа на старую. Скорее всего будет много приложений, кто использует старый и новый компонент, и по опыту взаимодействия они будут отличаться. Было бы здорово, если пользователь бы не понял подмены.

Кажется еще чуть чуть требуется калибровка. Опираться можно на пример нативных модалок в vk клиентах

@shevchux
Copy link

shevchux commented Feb 26, 2024

на ios очень палится что это фейковый скролл, тяжелее скроллить, опираясь на инерцию

@shevchux
Copy link

На iOS резком скролле вниз фиксированная кнопка эластично подпрыгивает и возвращается обратно.

@shevchux
Copy link

shevchux commented Feb 26, 2024

В старой реализации полноэкранная модалка была с отступом, чтобы была видна шапка родительской панели.

image

Вижу такое же поведение в нативном компоненте iOS клиента: просмотр инфо о своем профиле.

@shevchux
Copy link

Ок что при взаимодействии с высотой небольшой модалки меняется степень прозрачности фона? Раньше степень прозрачности фона менялась только при открытии и закрытии модалки. ИМХО Измена фона в процессе взаимодействия с модалкой чуть отвлекает

image image

@SevereCloud
Copy link
Contributor Author

Ок что при взаимодействии с высотой небольшой модалки меняется степень прозрачности фона?

Поправил 0ad8fad

@mendrew
Copy link
Contributor

mendrew commented Feb 28, 2024

[1]

закрыть модалку потянув вниз лишь заголовок

Не нашел ничего нативного, что имело бы такое же поведение. Да и реализовывать такое очень не хочется

Действительно, похоже я перепутал с кастомным паттерном когда показывается информационное сообщение снизу с помощью компонента (Sheet), а при выдвигании контента появляются кнопки для действия. Там почему-то именно за dragger элемент нужно было держаться для выдвигания и скрытия. Но это похоже, что просто тонкости реализации и хотелки дизайна.

Тогда просто интересует ответ на вопрос, который Вика задала. [2]

А это требование какое-то, чтобы при закрытии модалка проскролливалась вверх и только потом закрывалась или баг?)

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

Спрашиваю не фикса ради, а просто поднимаю для обсуждения. Может быть потом к нам с этим придут.

@SevereCloud
Copy link
Contributor Author

В старой реализации полноэкранная модалка была с отступом, чтобы была видна шапка родительской панели.

Поправил поддержку hasCustomPanelHeaderAfter 83546b1

@SevereCloud
Copy link
Contributor Author

На iOS резком скролле вниз фиксированная кнопка эластично подпрыгивает и возвращается обратно.

Не воспроизвел

@SevereCloud
Copy link
Contributor Author

на ios очень палится что это фейковый скролл, тяжелее скроллить, опираясь на инерцию

Это нативный скролл. Скролл может затормаживаться(scroll-snap-stop: always) при приближении к началу модалки, чтобы случайно не закрыть ее

@SevereCloud
Copy link
Contributor Author

Анимация открытия и закрытия медленная кажется, не похожа на старую.

Под капотом используется нативный el.scrollTo({top: ...,behavior: 'smooth',}), к сожалению скоростью нельзя управлять

@BlackySoul
Copy link
Contributor

А что по итогу с этим? Будем ссылаться на особенности реализации?

Ещё в доке на айпаде ловлю вот такое странное поведение, при проскролливании списка он в какой-то момент мигает и сбрасывается в начало:

6209058835129.mp4

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

6209077119673.mp4

@shevchux
Copy link

А что по итогу с этим? Будем ссылаться на особенности реализации?

Ещё в доке на айпаде ловлю вот такое странное поведение, при проскролливании списка он в какой-то момент мигает и сбрасывается в начало:

6209058835129.mp4
И вообще в доке на айпаде ощущается очень багованным скрол (с динамической высотой он как будто подтормаживает и откликается через секунду, в фильтрах мигает постоянно):

6209077119673.mp4

Поддерживаю, вопрос актуален. Взаимодействие плохо мимикрирует под нативное, как в VK клиенте. Я бы в таком виде не распространял компонент в общее использование. Простите

@shevchux
Copy link

Сепаратор шапки должен прятаться при скролле контента вниз

@shevchux
Copy link

Боковые отступы у сепараторов должны быть побольше. Нужно взять токен горизонтального отступа

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-stale Добавляет PR в исключения для автоматического закрытия
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Переписать модальные окна
6 participants