-
Notifications
You must be signed in to change notification settings - Fork 185
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
base: master
Are you sure you want to change the base?
feat: add ModalSheet #5092
Conversation
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 ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
d9253b1
to
d0d0404
Compare
e2e tests |
....неужели... неужели модалки начинают путь к рефактору 🥲 ✨💖✨ Правильно понимаю, что от Глянул на iOS, вот что заметил:
Перенёс пример в песочницу https://codesandbox.io/s/pr5092-modalpagenew-438xlz, чтобы было удобней проверять на мобилках. |
Хотелось бы, чтобы модальные окна и попапы работали без SplitLayout. Если отказываться от ModalRoot, нужно предложить какую-то систему по открытию нескольких модалок (см #2449)
В сторибуке можно открывать в отдельном окне |
Найс Про систему открытия надо будет покумекать, ну и как в issue написано, нужно ещё с дизайном обсудить
да, просто в codesandbox можно более гибко потыкать, в сторибуке же всё зашито |
d0d0404
to
7ab238c
Compare
7ab238c
to
596ada9
Compare
👀 Docs deployed
Commit 83546b1 |
Привет! В примере на styleguide с динамической высотой при ширине 320 заметил, что при скроллинге модалка не закрывается, а при раскрытии Accordion-а скроллинг закрывает модалку а не видвигает ее. |
Фикс |
There was a problem hiding this 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, есть вот такие замечания:
- В
ModalFirst
при раскрытии на весь экран, не выключаетсяscroll-snap
(см. Видео). - В
ModalDynamic
не пересчитывается высота. (см. Видео). - Можно ли сделать так, чтобы при скролле сверху вниз, когда мы проскролили контент, модалка не закрывалась сразу? (см.
ModalFull
в см. Видео).
Видео
RPReplay_Final1703068228.MP4
packages/vkui/src/components/ModalSheet/components/Indent/Indent.tsx
Outdated
Show resolved
Hide resolved
Не очень понял, а зачем выключать
Оно уже есть, но оно не реагирует если сильно скролить |
Можно на видео наблюдать, что скролл сегментирован, как-будто слайды листаю Хочется такой же прокрутки как в |
Возможно это кажется из-за обязательной остановки при закрытии модалки. Там просто не может быть снапа |
57be80d
to
cec5cc7
Compare
А это требование какое-то, чтобы при закрытии модалка проскролливалась вверх и только потом закрывалась или баг?) 2023-12-21.20.59.50.mov |
Co-authored-by: Andrey Medvedev <andr.medv.spb@gmail.com>
7857084
to
355e9b5
Compare
Не нашел ничего нативного, что имело бы такое же поведение. Да и реализовывать такое очень не хочется |
Анимация открытия и закрытия медленная кажется, не похожа на старую. Скорее всего будет много приложений, кто использует старый и новый компонент, и по опыту взаимодействия они будут отличаться. Было бы здорово, если пользователь бы не понял подмены. Кажется еще чуть чуть требуется калибровка. Опираться можно на пример нативных модалок в vk клиентах |
на ios очень палится что это фейковый скролл, тяжелее скроллить, опираясь на инерцию |
На iOS резком скролле вниз фиксированная кнопка эластично подпрыгивает и возвращается обратно. |
Поправил 0ad8fad |
Действительно, похоже я перепутал с кастомным паттерном когда показывается информационное сообщение снизу с помощью компонента (Sheet), а при выдвигании контента появляются кнопки для действия. Там почему-то именно за Тогда просто интересует ответ на вопрос, который Вика задала. [2]
Могу представить как в модалке используется список файлов, которых может быть миллион, как в почте, например (с виртуальным список это может и не проблема). Может быть просто очень длинный текст. Странно проскроливать сквось всё обратно. Хотя может быть я ошибаюсь. Спрашиваю не фикса ради, а просто поднимаю для обсуждения. Может быть потом к нам с этим придут. |
Поправил поддержку |
Не воспроизвел |
Это нативный скролл. Скролл может затормаживаться(scroll-snap-stop: always) при приближении к началу модалки, чтобы случайно не закрыть ее |
Под капотом используется нативный |
А что по итогу с этим? Будем ссылаться на особенности реализации? Ещё в доке на айпаде ловлю вот такое странное поведение, при проскролливании списка он в какой-то момент мигает и сбрасывается в начало: 6209058835129.mp4И вообще в доке на айпаде ощущается очень багованным скрол (с динамической высотой он как будто подтормаживает и откликается через секунду, в фильтрах мигает постоянно): 6209077119673.mp4 |
Поддерживаю, вопрос актуален. Взаимодействие плохо мимикрирует под нативное, как в VK клиенте. Я бы в таком виде не распространял компонент в общее использование. Простите |
Сепаратор шапки должен прятаться при скролле контента вниз |
Новый компонент ModalSheet, который использует
scroll-snap
вместо прошлого способаПотенциально избавляет от следующих проблем: