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

refactor: ModalRoot/ModalPage/ModalCard #6759

Merged
merged 9 commits into from
Dec 2, 2024

Conversation

inomdzhon
Copy link
Contributor

@inomdzhon inomdzhon commented Mar 26, 2024

Проверить после


  • Unit-тесты
  • e2e-тесты
  • [ ] Дизайн-ревью – визуально не компонент не затрагивал, поменял только поведение, к сожалению, поведение дизайн пока не документирует.
  • Документация фичи
  • Release notes

Описание

Переписаны компоненты ModalPage/ModalCard

⚠️ Теперь компоненты могут использоваться без ModalRoot.

ModalPage / ModalCard:

  • добавлено свойство open;
  • добавлено свойство keepMounted;
  • типа onClose изменён с VoidFunction на (reason: ModalPageCloseReason, event?: UIEvent<HTMLElement>) => void;
  • добавлено свойство noFocusToDialog, приоритетней чем noFocusToDialog в ModalRoot;
  • добавлено свойство modalOverlayTestId, приоритетней чем modalOverlayTestId в ModalRoot.
  • создан компонент ModalOutlet;
  • создан компонент ModalOverlay;
  • основной код работы модалок вынесен в отдельные компоненты ModalPageInternal и ModalCardInternal, у них есть свойство ModalOverlay;
  • создан контекст ModalContext, который теперь используется в компонентах ModalPageHeader, Group и PanelHeader вместо ModalRootContext.

ModalPage:

  • settlingHeight теперь имеет значение по умолчанию 50% вместо 75%, обратил внимание, что в обычно BottomSheet'ы открываются на половину экрана;
  • добавлено свойство footer, а также создан компонент ModalPageFooter;
  • созданы компонент ModalPageContent – вынес, чтобы можно было в будущем перейти на сбор ModalPage через композицию компонентов.

lib/sheet:

В папке хранится логика отвечающая за взаимодействие с модалкой на мобильных экранах. Отдаёт хук useBottomSheet().

Переписан компонент ModalRoot

⚠️ Теперь лишь отвечает за состояние open компонентов ModalCard и ModalPage в зависимости от их id/nav и параметра activeModal, а также рендера общей ModalOverlay для всех модалок.

ModalRoot:

  • добавлены события onOpen, onOpened, onClosed, которые всплывают от activeModal;
  • в PopoutRoot удалён PopoutRootModal в пользу ModalOutlet у ModalPage и ModalCard.

ModalRootContext:

  • ⚠️ BREAKING CHANGE в свойство onClose нужно теперь обязательно передавать id модального окна;
  • добавлены события onOpen, onOpened, onClosed, чтобы их могли вызывать ModalPage и ModalCard;
  • свойство registerModal теперь @deprecated – не нужно отдельно регистрировать модальное окно.
  • свойство updateModalHeight теперь @deprecated – задача с обновлением высоты контента при dynamicContentHeight решается через CSS;

ModalRootOverlayContext / VisuallyHiddenModalOverlay:

  • чтобы создать общий ModalOverlay для всех модалок в контексте ModalRoot, происходит подмена ModalOverlay в ModalPage и ModalCard на VisuallyHiddenModalOverlay, который отвечает за приём onClick и modalOverlayTestId, а сам ModalOverlay попадает в начало ModalRoot

useModalManager():

  • отвечает за unmounted состояние;
  • отвечает за регулирования приоритета параметров из ModalRoot и ModalPage/ModalCard;
  • отвечает за подмену ModalOverlay на VisuallyHiddenModalOverlay.

withModalRootContext:

  • ввиду отказа от updateModalHeight HOC тоже @deprecated.

Изменения, которые нужно вынести в отдельные PR после первичного ревью

Нюансы

Обратная совместимость

Постарался сделать так, чтобы миграция прошла бесследно.

Сломаются вот такой кейс:

const MyModal = ({ id }) => { // пропустили settinglingHeight / dynamicContentHeight
  return <ModalPage>Lorem Ipsum</ModalPage>
};

const App = () => (
  <ModalRoot>
    <MyModal
      id="example-1"
      settinglingHeight={100} // устанавливалось здесь, т.к. раньше ModalRoot итерировал по потомкам и доставал этот параметр
    />
    
    <MyModal
      id="example-2"
      dynamicContentHeight // устанавливалось здесь, т.к. раньше ModalRoot итерировал по потомкам и доставал этот параметр
    />
  </ModalRoot>
);

который нужно будет править руками.

BottomSheet, анимации и свайп

Полное появление и полное скрытие происходит через transform, но анимация взаимодействия через свайп реализована через height, т.к. это оказалось самым оптимальным способом для решения задач:

  • закреплённый ModalPageFooter внизу;
  • возможность скроллить при settlingHeight меньше 100;
  • обновление высоты, если задан dynamicContentHeight.

Нашёл решение допустимым, т.к. свайп используется либо для закрытия, либо для разворачивания/сворачивания модального окна на всю или на половину страницы. В первом случае сработает закрытие через transform, а во втором разворачивание/сворачивание произойдёт через height.

dynamicContentHeight

см. предыдущий пункт про BottomSheet для контекста

Обновление высоты происходит без анимации, т.к. height: auto не анимируется. Опустил анимирование, т.к. усложняет компонент. В теории можно прибегнуть к useResizeObserver().

Адативность

При platform="vkcom" и при разрешении экрана 767px компонент теперь превращается в BottomSheet, но при этом логику взаимодействия через тач не имеет, т.к. isDesktop при platform="vkcom" всегда true вне зависимости от размера экрана.

Решения

Выделение текста 🔗

С помощью функции hasSelectionWithRangeType определяем, что пользователь выделил текст и перестаём реагировать на touchstart и touchmove пока выделение не будет удалено.

Вертикальный и горизонтальный скроллы 🔗

  • вертикальный скролл::
    • основной скролл (ModalPageContent): проверяем на scrollTop !== 0
    • другие скроллы: при touchstart достаём скроллируемый элемент через event.target и проверяем на положение scrollTop !== 0 и направление пальца вверх
  • горизонтальный скролл: не блочится, т.к. реагируем на события только по оси Y.

Плавающие элементы внутри модалки 🔗

Нужно рекомендовать использовать forcePortal – в коде проверяем, что идёт взаимодействие с элементом вне модалки.

Или нужно рекомендовать добавлять в корневой элемент плавающего элемента атрибут data-vkui-prevent-swipe .

Поля ввода 🔗

Наилучшего варианта не нашёл кроме как:

  1. через useVirtualKeyboardState() узнавать, что пользователь работает с клавиатурой, и перебивать safe-area-inset-bottom на тот, что возвращает хук 0 (попытка вычислять разницу высоты через VisualViewport, чтобы реагировать на смену размера клавиатуры, например, из-за панели эмодзи, не удалась);
  2. в том же хуке слушать событие скролла на window и сохранять его позицию на window.scrollTo(0, visualViewport.offsetTop).

Так как иные решения приводят к другим проблемам (подробнее можно прочесть в JSDoc хука useVirtualKeyboardState()), следующие баги нужно закрыть:

Референсы

Release notes

BREAKING CHANGE

  • ModalRoot: удалена реализация контекста через React.cloneElement, которая требовала передавать settlingHeight и dynamicContentHeight в обёртки над ModalPage / ModalCard.

    Пример миграции (перенос `settlingHeight` / `dynamicContentHeight`)
    const SomeWrapper = ({ id }) => (
      <ModalPage
        id={id}
    +   settlingHeight={100} // или dynamicContentHeight
      />
    );
    
    <ModalRoot activeModal="m">
      <SomeWrapper
        id="m"
    -   settlingHeight={100} // или dynamicContentHeight
      />
    </ModalRoot>
    Пример миграции (пробрасывание `settlingHeight` / `dynamicContentHeight`)
    - const SomeWrapper = ({ id }) => (
    + const SomeWrapper = (props) => (
      <ModalPage
    -   id={id}
    +   {...props}
      />
    );
    
    <ModalRoot activeModal="m">
      <SomeWrapper
        id="m"
        settlingHeight={100} // или dynamicContentHeight
      />
    </ModalRoot>

Улучшения

  • ModalRoot:
    • updateModalHeight() помечен как @depreacted, т.к. в нём больше нет необходимости – ModalPage, при dynamicContentHeight, теперь автоматически подстраиваются под контент;
    • registerModal() помечен как @depreacted, т.к. изменилась логика работы компонента – теперь ModalPage и ModalCard ориентируется на контекст, создаваемый ModalRoot;
    • добавлено свойство usePortal.
  • ModalPage:
    • теперь можно использовать без ModalRoot (для рендера в портале можно обернуть в AppRootPortal);
    • изменилось значение по умолчанию у свойств settlingHeight7550 ;
    • добавлено свойство keepMounted;
    • добавлено свойство footer;
    • добавлено свойство disableContentPanningGestureProp;
    • расширен тип onClose до (reason: ModalPageCloseReason, event?: UIEvent<HTMLElement>) => void.
  • ModalCard:
    • теперь можно использовать без ModalRoot (для рендера в портале можно обернуть в AppRootPortal);
    • добавлено свойство keepMounted;
    • расширен тип onClose до (reason: ModalPageCloseReason, event?: UIEvent<HTMLElement>) => void.

Copy link
Contributor

github-actions bot commented Mar 26, 2024

size-limit report 📦

Path Size
JS 384.63 KB (-0.25% 🔽)
JS (gzip) 116.33 KB (-0.45% 🔽)
JS (brotli) 95.78 KB (-0.41% 🔽)
JS import Div (tree shaking) 1.56 KB (0%)
CSS 337.07 KB (+0.64% 🔺)
CSS (gzip) 42.75 KB (+0.88% 🔺)
CSS (brotli) 33.77 KB (+1.04% 🔺)

Copy link

codesandbox-ci bot commented Mar 26, 2024

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 49b82a4:

Sandbox Source
vkui boilerplate (forked) Issue #1494

Copy link
Contributor

github-actions bot commented Mar 26, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Mar 26, 2024

👀 Docs deployed

Commit 49b82a4

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 95.86207% with 24 lines in your changes missing coverage. Please review.

Project coverage is 95.49%. Comparing base (95b518c) to head (49b82a4).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...kui/src/components/ModalPage/ModalPageInternal.tsx 87.83% 9 Missing ⚠️
...ckages/vkui/src/components/ModalPage/ModalPage.tsx 85.71% 6 Missing ⚠️
...kui/src/components/ModalCard/ModalCardInternal.tsx 92.15% 4 Missing ⚠️
...src/lib/sheet/controllers/BottomSheetController.ts 98.97% 2 Missing ⚠️
packages/vkui/src/lib/sheet/index.ts 71.42% 2 Missing ⚠️
.../vkui/src/components/ModalOverlay/ModalOverlay.tsx 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6759      +/-   ##
==========================================
+ Coverage   95.45%   95.49%   +0.03%     
==========================================
  Files         384      393       +9     
  Lines       11312    11188     -124     
  Branches     3780     3708      -72     
==========================================
- Hits        10798    10684     -114     
+ Misses        514      504      -10     
Flag Coverage Δ
unittests 95.49% <95.86%> (+0.03%) ⬆️

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.

@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Apr 3, 2024
@inomdzhon inomdzhon added the no-stale Добавляет PR в исключения для автоматического закрытия label Apr 6, 2024
@inomdzhon inomdzhon force-pushed the imirdzhamolov/research/refactor-modal branch 3 times, most recently from 4a0d455 to 7d03282 Compare May 16, 2024 16:03
@inomdzhon inomdzhon force-pushed the imirdzhamolov/research/refactor-modal branch 2 times, most recently from cd0da9c to c382b8f Compare August 1, 2024 14:58
@inomdzhon inomdzhon force-pushed the imirdzhamolov/research/refactor-modal branch from c382b8f to fba65d5 Compare September 30, 2024 09:04
@inomdzhon inomdzhon force-pushed the imirdzhamolov/research/refactor-modal branch 3 times, most recently from c1453c5 to a1c9447 Compare October 28, 2024 23:33
@inomdzhon inomdzhon changed the title [Draft] feat: create ModalPageV2 refactor: ModalRoot/ModalPage/ModalCard Oct 28, 2024
@inomdzhon inomdzhon force-pushed the imirdzhamolov/research/refactor-modal branch 6 times, most recently from 481fc96 to f07b8b4 Compare October 30, 2024 15:25
@inomdzhon inomdzhon marked this pull request as ready for review October 30, 2024 15:25
@inomdzhon inomdzhon requested a review from a team as a code owner October 30, 2024 15:25
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.

Грандиозная работа! 👏 👏 👏 💪

Здорово раскидал по компонентам!
Понравилось, что компоненты теперь функции как практически везде!
Офигенно как переведена анимация/перетаскивание на CSS Transition и контроллеры. (Там я, конечно, поплыл, но приятно видеть знакомое API с коллбэками и то, что всё сводится к установке css переменных)


Надо пристально пройтись по местам где коллбёки передаются в компоненты, а не напрямую в html элементы и обернуть их useCallback, и по местам, где объекты возвращаются из хуков или передаются в контекст, чтобы обернуть в useMemo и избежать лишних ререндеров.


По стайлгайду и сторибуку прошелся и так и на устройстве, ничего такого не заметил.

packages/vkui/src/components/ModalRoot/ModalRoot.tsx Outdated Show resolved Hide resolved
packages/vkui/src/hooks/useFocusTrap.ts Outdated Show resolved Hide resolved
packages/vkui/src/hooks/useVirtualKeyboardState.ts Outdated Show resolved Hide resolved
packages/vkui/src/lib/dom.tsx Outdated Show resolved Hide resolved
@inomdzhon
Copy link
Contributor Author

Надо пристально пройтись по местам где коллбёки передаются в компоненты, а не напрямую в html элементы и обернуть их useCallback, и по местам, где объекты возвращаются из хуков или передаются в контекст, чтобы обернуть в useMemo и избежать лишних ререндеров.

добавил мемоизацию там где можно, но во всех местах сильная завязка на пользователя, т.к. используются колбеки (onOpen, onOpened, onClose, onClosed), которые пользователь сам должен мемоизировать

ждём React 19, где не нужно будет руками мемоизировать)

@inomdzhon inomdzhon mentioned this pull request Nov 1, 2024
1 task
@BlackySoul
Copy link
Contributor

Ещё заметила, что как будто пропала фича, которая позволяла высоту модалки "запоминать". Т.е. если мы вытянули первую модалку на всю высоту, переключились на другую и вернулись на первую - первая должна сохранить высоту

@inomdzhon
Copy link
Contributor Author

inomdzhon commented Nov 29, 2024

Ещё заметила, что как будто пропала фича, которая позволяла высоту модалки "запоминать". Т.е. если мы вытянули первую модалку на всю высоту, переключились на другую и вернулись на первую - первая должна сохранить высоту

воу, действительно, пропала...

  • 8275749 – немного переделал логику в BottomSheetController (изначально не было необходимости сразу так сделать)
  • 0346909 – а в этом коммите накидал логику сохранения состояния (🔗 Storybook, 🔗 Styleguide), но ух... получилось как-будто сложно 🥲 мб опустим? 😅 вообще по дизайну модалки должны друг на друга накладываться, а на iOS, при раскрытии на 100% эффект стека должен быть – отложил это на закуску после рефактора
    image

BlackySoul
BlackySoul previously approved these changes Dec 2, 2024
@inomdzhon inomdzhon force-pushed the imirdzhamolov/research/refactor-modal branch from 0346909 to 49b82a4 Compare December 2, 2024 07:27
@inomdzhon inomdzhon merged commit 1234239 into master Dec 2, 2024
28 checks passed
@inomdzhon inomdzhon deleted the imirdzhamolov/research/refactor-modal branch December 2, 2024 09:01
inomdzhon added a commit that referenced this pull request Dec 11, 2024
Зарезолвил TODO в тесте `useBottomSheet()`, который оставлял в #6759

Упростил `getFakeTouchEvent` и `getFakeMouseEvent` до функций, которые возвращают объект.

Заметил, что если вторым аргументом в `fireEvent` из `testing-library/react` передавать объект события, то он работает не корректно – лучше передавать сразу опции, которые принимают те или иные события.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment