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(ValidationContextWrapper): add virtualized wrapper #3578

Open
wants to merge 11 commits into
base: next
Choose a base branch
from

Conversation

Critonik
Copy link

@Critonik Critonik commented Dec 24, 2024

Проблема

Валидации работают только с элементами, смонтированными в dom-дерево. Если есть данные, что невалидны по правилам валидации, но их не существует - форма будет считаться валидной. Проблемы появляются с отрисовкой больших списков. Если использовать виртуализацию, то неотрисованные данные не отвалидируются и данные, потенциально, сохранятся с ошибкой.

Решение

ValidationBuilder при создании сразу валидирует данные по правилам, переданным в него. У ValidationContainer\ValidationWrapper нет доступа явного к билдеру. Идея состоит в том, что мы передаем явно массив node, используемых для отрисовки строк\таблиц и читаем вглубь через ValidationReader до первого состояния error\warning и возвращаем индекс строки. Специальный кмопонент ValidationWrapperVirtualizedInternal при валидации передает этот индекс и сохраняет знание о том, что внутри него какой-то элемент невалиден.

Чек-лист перед запросом ревью

  1. Добавлены тесты на все изменения
    ⬜ unit-тесты для логики
    ⬜ скриншоты для верстки и кросс-браузерности
    ✅ нерелевантно

  2. Добавлена (обновлена) документация
    ✅ styleguidist для пропов и примеров использования компонентов
    ⬜ jsdoc для утилит и хелперов
    ⬜ комментарии для неочевидных мест в коде
    ⬜ прочие инструкции (README.md, contributing.md и др.)
    ⬜ нерелевантно

  3. Изменения корректно типизированы
    ⬜ без использования any (см. PR 2856)
    ✅ нерелевантно

  4. Прочее
    ⬜ все тесты и линтеры на CI проходят
    ✅ в коде нет лишних изменений
    ⬜ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)

@Critonik Critonik self-assigned this Dec 24, 2024
@Critonik Critonik marked this pull request as ready for review December 25, 2024 08:56
@Critonik Critonik requested a review from lossir December 25, 2024 10:56
@mshatikhin mshatikhin requested a review from zhzz December 26, 2024 09:24
@zhzz
Copy link
Member

zhzz commented Jan 29, 2025

Я расширил пример в документации, добавив реальных инпутов, разные виды валидации и форму без виртуализации для сравнения. Он доступен по ссылке.

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

В текущем поведении есть отличия в виртуальном списке:

  1. при валидации по сабмиту после нажатия кнопки поля загораются не с первого раза
  2. при скроле после валидации любого типа, кроме immediate, поля гаснут после перерисовки

Такого не должно быть и это стоит устранить в любом случае.

Далее опишу предложения для идеального решения. Они могут отпасть в ходе решения проблем выше, или наоборот, стать более актуальными. Поэтому, их можно рассмотреть во вторую очередь.

  1. Сейчас ValidationListWrapper принимает в проп validationInfos только ValidationReader<any[]>. Это привязывает его к работе только через ValidationBuilder, но он не является обязательным к использованию в библиотеке. Получается ограничение. Более общим вариантом было бы работать напрямую с Validation[]. Структура данных все равно вроде как известна заранее и список по большому счету плоский, основанный на индексах. Работу с ValidationBuilder тоже можно предусмотеть, добавив ему возможность отдавать Validation[].

  2. Другая особенность сейчас в том, что ValidationWrapper-ы, рисуясь внутри ValidationListWrapper, продолжают регистрироваться и взаимодействовать напрямую с ValidationContainer. При этом за них уже частично отвечает ValidationListWrapper. Получается, что ValidationContainer дергает одни и те же методы и у ListWrapper и у Wrapper внутри ListWrapper. Но на деле реально работают методы только у кого-то одного из них, и не совсем очевидно у кого какие. Конкретных проблем с этим на практике я пока не обнаружил, но они вполне вероятны. В любом случае было бы прозрачнее с точки зрения логики, если бы ValidationWrapper-ы внутри ValidationListWrapper напрямую бы не взаимодействовали больше с ValidationContainer, а за них бы полностью отвечал ValidationListWrapper.

  3. Пропы level, behaviour и independent в ValidationListWrapper. Эти настройки на самом деле правильно задавать через validationInfos. В пропах листа они нужны, только в виде заглушки, но при этом влияют на результат hasError/hasWarning у ValidationListWrapper. И когда значение пропа не совпадет с тем, что в validationInfos, то результат валидации всей формы будет неправильный.

@zhzz
Copy link
Member

zhzz commented Feb 4, 2025

Заметил еще проблему, которую точно нужно решить, это фокус на поле с ошибкой по сабмиту. Сейчас в листе он не происходит. Думаю, нужно каким-то образом научиться находить нужный ValidationWrapper и вызывать метод focus у него, т.к. в нем есть нужная дополнительная логика.

Проблему с динамическим переключением типа валидации и перемонтированием элементов внутри листа, которую еще обсуждали, предлагаю пока тоже отложить.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants