Alexey-Timakov / react-login-form

0 stars 0 forks source link

Ревью по проекту react-login-form #1

Open Lebed-kun opened 2 years ago

Lebed-kun commented 2 years ago
  1. https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L73

Регулярки кмк хорошо подходят для паттерн-матчинга подстрок (например что имейл соответствует корректному формату). Наличие определенных символов в строке я бы проверял обычным алгоритмом с циклом и несколькими флагами (ну или бы метчил относительно нескольких регулярок с character sets): какую он логику отражает, понять кмк проще чем суп из lookahead-условий;

2. https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L24

Я бы завернул подобные выражения в useMemo, чтобы не создавать объекты и конвертировать их поток в строки на каждый рендер компонента (а только когда меняются нужные флаги);

3. https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L116

Зачем нужно менять свойство императивно через реф когда можно менять реактивно через свойство disabled у кнопки? На мой взгляд, рефы хороши в двух случаях:

1) Когда нужно произвести тонкие UI-манипуляции над элементами, для которых использование цикла обновления React покажется оверхедом. Например, когда нужно менять зум у интерактивной карты при прокрутке колесика мыши. Или какая-нибудь Реакт-обертка над физической симуляцией. И пр. 2) Когда UI-состояние компонента по задумке элегантнее менять через прямой вызов функции. Например, закрыть зарендеренную модалку через какую-нибудь кнопку внутри ее контента;

Ну и еще хук useRef дает контейнер для нереактивного состояния для функциональных компонентов (вместо полей у классов);

4. https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L101

Можно сразу возвращать false;

5. https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L132

Незаскейлится если по бизнес-логике предполагается не только один вид ошибки для поля;

Alexey-Timakov commented 2 years ago

https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L73

Регулярки кмк хорошо подходят для паттерн-матчинга подстрок (например что имейл соответствует корректному формату). Наличие определенных символов в строке я бы проверял обычным алгоритмом с циклом и несколькими флагами (ну или бы метчил относительно нескольких регулярок с character sets): какую он логику отражает, понять кмк проще чем суп из lookahead-условий;

https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L24

Я бы завернул подобные выражения в useMemo, чтобы не создавать объекты и конвертировать их поток в строки на каждый рендер компонента (а только когда меняются нужные флаги);

https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L116

Зачем нужно менять свойство императивно через реф когда можно менять реактивно через свойство disabled у кнопки? На мой взгляд, рефы хороши в двух случаях:

  1. Когда нужно произвести тонкие UI-манипуляции над элементами, для которых использование цикла обновления React покажется оверхедом. Например, когда нужно менять зум у интерактивной карты при прокрутке колесика мыши. Или какая-нибудь Реакт-обертка над физической симуляцией. И пр.
  2. Когда UI-состояние компонента по задумке элегантнее менять через прямой вызов функции. Например, закрыть зарендеренную модалку через какую-нибудь кнопку внутри ее контента;

Ну и еще хук useRef дает контейнер для нереактивного состояния для функциональных компонентов (вместо полей у классов);

https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L101

Можно сразу возвращать false;

https://github.com/Alexey-Timakov/react-login-form/blob/main/src/Form.tsx#L132

Незаскейлится если по бизнес-логике предполагается не только один вид ошибки для поля;

Спасибо за комментарий! Тут все не так серьезно, конечно, допиливать можно сколько угодно. Часть всего придумывалась на ходу и переписывать старый код уже не считал необходимым.

По поводу пароля (почему некорректно работает) надо посмотреть. Я обычно использовал библиотеку validator, но тут решил обойтись. Надо тесты писать конечно :) С useMemo пока не довелось работать. Но учту. Имеется ввиду btn.disabled = tryue / false? Так не получилось сделать - вылезала ошибка по типам. Победил вот именно таким образом, как оно сделано. В других проектах такого не встречал еще. Наверное настройки TS влияют, не знаю. Да, согласен. Тут все сделано просто. Данная функция добавлена уже в последнюю очередь, чтобы показать, почему кнопка submit не работает.

Lebed-kun commented 2 years ago

Так не получилось сделать - вылезала ошибка по типам

Странно. checkIsValuesFilled возвращает же boolean;

Я обычно использовал библиотеку validator, но тут решил обойтись

В одном моем проекте вся логика для работы с формой (включая валидацию полей) умещалась в несколько десятков строк кода в один красивенький хук. Не так оптимально обновляются стейты по ошибкам и полям формы конечно в отличие от final-form (сорцы не изучал, подозреваю что там в объекте для стейта формы используются события типа onFieldChange{ name: string, value: any; }), но всё же: https://gitlab.com/john-byte/my-anki/-/blob/master/frontend/src/hooks/form.ts